[PATCH] D122909: [InstCombine] limit icmp fold with sub if other sub user is a phi

[Sanjay Patel via Phabricator via llvm-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5f8c2b884d42: [InstCombine] limit icmp fold with sub if other sub user is a phi (authored by spatel).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122909/new/

https://reviews.llvm.org/D122909

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


Index: llvm/test/Transforms/InstCombine/icmp-sub.ll
===================================================================
--- llvm/test/Transforms/InstCombine/icmp-sub.ll
+++ llvm/test/Transforms/InstCombine/icmp-sub.ll
@@ -502,7 +502,7 @@
   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 @@
 ; 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 @@
   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 @@
 ; 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
Index: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -2593,13 +2593,19 @@
 
   // 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;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D122909.420020.patch
Type: text/x-patch
Size: 2929 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220402/f96cdcac/attachment.bin>