[llvm] 5f8c2b8 - [InstCombine] limit icmp fold with sub if other sub user is a phi

[Sanjay Patel via llvm-commits]

Author: Sanjay Patel
Date: 2022-04-02T19:23:42-04:00
New Revision: 5f8c2b884d4288617138114ebd2fdb235452c8ce

URL: https://github.com/llvm/llvm-project/commit/5f8c2b884d4288617138114ebd2fdb235452c8ce
DIFF: https://github.com/llvm/llvm-project/commit/5f8c2b884d4288617138114ebd2fdb235452c8ce.diff

LOG: [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

Added: 
    

Modified: 
    llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
    llvm/test/Transforms/InstCombine/icmp-sub.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 4c34d56526ffe..39c20ec06ca29 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<PHINode>(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 026560d5c6da2..74d002fefda8d 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