[PATCH] D72864: [InstCombine] Fix worklist management in return combine

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added reviewers: spatel, lebedev.ri.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

There are two related bugs here: First, we don't add the operand we're replacing to the worklist, which means it may not get DCEd (see test change). Second, usually this would just get picked up in the next iteration, but we also do not report the instruction as changed. This means that we don't get that extra instcombine iteration, and more importantly, may break the pass pipeline, as the function is not marked as changed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72864

Files:
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/test/Transforms/InstCombine/assume.ll


Index: llvm/test/Transforms/InstCombine/assume.ll
===================================================================
--- llvm/test/Transforms/InstCombine/assume.ll
+++ llvm/test/Transforms/InstCombine/assume.ll
@@ -190,16 +190,10 @@
 }
 
 define i32 @icmp1(i32 %a) #0 {
-; EXPENSIVE-ON-LABEL: @icmp1(
-; EXPENSIVE-ON-NEXT:    [[CMP:%.*]] = icmp sgt i32 [[A:%.*]], 5
-; EXPENSIVE-ON-NEXT:    tail call void @llvm.assume(i1 [[CMP]])
-; EXPENSIVE-ON-NEXT:    ret i32 1
-;
-; EXPENSIVE-OFF-LABEL: @icmp1(
-; EXPENSIVE-OFF-NEXT:    [[CMP:%.*]] = icmp sgt i32 [[A:%.*]], 5
-; EXPENSIVE-OFF-NEXT:    tail call void @llvm.assume(i1 [[CMP]])
-; EXPENSIVE-OFF-NEXT:    [[CONV:%.*]] = zext i1 [[CMP]] to i32
-; EXPENSIVE-OFF-NEXT:    ret i32 1
+; CHECK-LABEL: @icmp1(
+; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt i32 [[A:%.*]], 5
+; CHECK-NEXT:    tail call void @llvm.assume(i1 [[CMP]])
+; CHECK-NEXT:    ret i32 1
 ;
   %cmp = icmp sgt i32 %a, 5
   tail call void @llvm.assume(i1 %cmp)
Index: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2592,14 +2592,17 @@
 
   Value *ResultOp = RI.getOperand(0);
   Type *VTy = ResultOp->getType();
-  if (!VTy->isIntegerTy())
+  if (!VTy->isIntegerTy() || isa<Constant>(ResultOp))
     return nullptr;
 
   // There might be assume intrinsics dominating this return that completely
   // determine the value. If so, constant fold it.
   KnownBits Known = computeKnownBits(ResultOp, 0, &RI);
-  if (Known.isConstant())
+  if (Known.isConstant()) {
+    Worklist.AddValue(ResultOp);
     RI.setOperand(0, Constant::getIntegerValue(VTy, Known.getConstant()));
+    return &RI;
+  }
 
   return nullptr;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D72864.238579.patch
Type: text/x-patch
Size: 1834 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200116/a9dffe16/attachment.bin>