From 5f8c2b884d4288617138114ebd2fdb235452c8ce Mon Sep 17 00:00:00 2001 From: Sanjay Patel Date: Sat, 2 Apr 2022 19:23:42 -0400 Subject: [PATCH] [InstCombine] limit icmp fold with sub if other sub user is a phi This is a hacky fix for: https://github.com/llvm/llvm-project/issues/54558 As discussed there, codegen regressed when we opened up this transform to allow extra uses ( 61580d0949fd3465 ), and it's not clear how to undo the transforms at the later stage of compilation. As noted in the code comments, there's a set of remaining folds that are still limited to one-use, so we can try harder to refine and expand the limitations on these folds, but it's likely to be an up-and-down battle as we find and overcome similar regressions. Differential Revision: https://reviews.llvm.org/D122909 --- llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp | 10 ++++++++-- llvm/test/Transforms/InstCombine/icmp-sub.ll | 8 +++++--- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp index 4c34d56..39c20ec 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp @@ -2593,13 +2593,19 @@ Instruction *InstCombinerImpl::foldICmpSubConstant(ICmpInst &Cmp, // X - Y == 0 --> X == Y. // X - Y != 0 --> X != Y. - if (Cmp.isEquality() && C.isZero()) + // TODO: We allow this with multiple uses as long as the other uses are not + // in phis. The phi use check is guarding against a codegen regression + // for a loop test. If the backend could undo this (and possibly + // subsequent transforms), we would not need this hack. + if (Cmp.isEquality() && C.isZero() && + none_of((Sub->users()), [](const User *U) { return isa(U); })) return new ICmpInst(Pred, X, Y); // The following transforms are only worth it if the only user of the subtract // is the icmp. // TODO: This is an artificial restriction for all of the transforms below - // that only need a single replacement icmp. + // that only need a single replacement icmp. Can these use the phi test + // like the transform above here? if (!Sub->hasOneUse()) return nullptr; diff --git a/llvm/test/Transforms/InstCombine/icmp-sub.ll b/llvm/test/Transforms/InstCombine/icmp-sub.ll index 026560d..74d002f 100644 --- a/llvm/test/Transforms/InstCombine/icmp-sub.ll +++ b/llvm/test/Transforms/InstCombine/icmp-sub.ll @@ -502,7 +502,7 @@ define i32 @sub_eq_zero_select(i32 %a, i32 %b, i32* %p) { ret i32 %sel } -; TODO: Replacing the "SUB == 0" regresses codegen, and it may be hard to recover from that. +; Replacing the "SUB == 0" regresses codegen, and it may be hard to recover from that. declare i32 @llvm.umin.i32(i32, i32) @@ -515,7 +515,7 @@ define void @PR54558_reduced(i32 %arg) { ; CHECK-NEXT: [[MIN:%.*]] = tail call i32 @llvm.umin.i32(i32 [[PHI_OUTER]], i32 43) ; CHECK-NEXT: call void @use(i32 [[MIN]]) ; CHECK-NEXT: [[SUB]] = sub i32 [[PHI_OUTER]], [[MIN]] -; CHECK-NEXT: [[COND_OUTER:%.*]] = icmp ult i32 [[PHI_OUTER]], 44 +; CHECK-NEXT: [[COND_OUTER:%.*]] = icmp eq i32 [[SUB]], 0 ; CHECK-NEXT: br i1 [[COND_OUTER]], label [[BB_EXIT:%.*]], label [[BB_LOOP]] ; CHECK: bb_exit: ; CHECK-NEXT: ret void @@ -535,6 +535,8 @@ bb_exit: ret void } +; TODO: It might be ok to replace the "SUB == 0" in this example if codegen can invert it. + define void @PR54558_reduced_more(i32 %x, i32 %y) { ; CHECK-LABEL: @PR54558_reduced_more( ; CHECK-NEXT: bb_entry: @@ -542,7 +544,7 @@ define void @PR54558_reduced_more(i32 %x, i32 %y) { ; CHECK: bb_loop: ; CHECK-NEXT: [[PHI_OUTER:%.*]] = phi i32 [ [[SUB:%.*]], [[BB_LOOP]] ], [ [[X:%.*]], [[BB_ENTRY:%.*]] ] ; CHECK-NEXT: [[SUB]] = sub i32 [[PHI_OUTER]], [[Y:%.*]] -; CHECK-NEXT: [[COND_OUTER:%.*]] = icmp eq i32 [[PHI_OUTER]], [[Y]] +; CHECK-NEXT: [[COND_OUTER:%.*]] = icmp eq i32 [[SUB]], 0 ; CHECK-NEXT: br i1 [[COND_OUTER]], label [[BB_EXIT:%.*]], label [[BB_LOOP]] ; CHECK: bb_exit: ; CHECK-NEXT: ret void -- 2.7.4