Const propagation after hitting assume bugfix 2

[Prazek]

Summary

There was infinite loop because it was trying to change assume(true) into assume(true)
Also added handling when assume(false) appear

[nlewycky]

Typo in summary, "There was ifinite loop" should say "infinite".

include/llvm/IR/Instructions.h

329 ↗

(On Diff #33661)

This change isn't logically part of the const propagation bugfix. Either commit it separately or discard it.

lib/Transforms/Scalar/GVN.cpp

1778–1780

GVN can directly modify the CFG (unlike InstCombine which calls setPreservesCFG). Rephrase the comment about unreachable instruction into a TODO.

[nlewycky]

lib/Transforms/Scalar/GVN.cpp

1779

Typo "cound" -> "could".

[nlewycky]

Forgot to write LGTM when I accepted revision.

[Prazek]

I heard that accepting revision is stronger that LGTM, so no worries, nobody greps for it (or do they?).

[reames]

FYI - LGTM is the 'official' sign off used by the community. We
frequently take an accepted phabricator revision to have been given an
LGTM, but that's not 'officially' okay. Doesn't really matter in
practice; I'm just being pedantic for the record.

lib/Transforms/Scalar/GVN.cpp

bool GVN::processNonLocalLoad(LoadInst *LI) {
 
   return PerformLoadPRE(LI, ValuesPerBlock, UnavailableBlocks);
 }
 
 bool GVN::processAssumeIntrinsic(IntrinsicInst *IntrinsicI) {
   assert(IntrinsicI->getIntrinsicID() == Intrinsic::assume &&
          "This function can only be called with llvm.assume intrinsic");
   Value *V = IntrinsicI->getArgOperand(0);
+
+  if (ConstantInt *Cond = dyn_cast<ConstantInt>(V)) {
+    if (Cond->isZero()) {
+      Type *Int8Ty = Type::getInt8Ty(V->getContext());
+      // Insert a new store to null instruction before the load to indicate that
+      // this code is not reachable.  FIXME: We cound insert unreachable
+      // instruction directly because we can modify the CFG.
+      new StoreInst(UndefValue::get(Int8Ty),
+                    Constant::getNullValue(Int8Ty->getPointerTo()),
+                    IntrinsicI);
+    }
+    markInstructionForDeletion(IntrinsicI);
+    return false;
+  }
+
   Constant *True = ConstantInt::getTrue(V->getContext());
   bool Changed = false;
+
   for (BasicBlock *Successor : successors(IntrinsicI->getParent())) {
     BasicBlockEdge Edge(IntrinsicI->getParent(), Successor);
 
     // This property is only true in dominated successors, propagateEquality
     // will check dominance for us.
     Changed |= propagateEquality(V, True, Edge, false);
   }
+
   // We can replace assume value with true, which covers cases like this:
   // call void @llvm.assume(i1 %cmp)
   // br i1 %cmp, label %bb1, label %bb2 ; will change %cmp to true
   ReplaceWithConstMap[V] = True;
 
   // If one of *cmp *eq operand is const, adding it to map will cover this:
   // %cmp = fcmp oeq float 3.000000e+00, %0 ; const on lhs could happen
   // call void @llvm.assume(i1 %cmp)

 // Tries to replace instruction with const, using information from
 // ReplaceWithConstMap.
 bool GVN::replaceOperandsWithConsts(Instruction *Instr) const {
   bool Changed = false;
   for (unsigned OpNum = 0; OpNum < Instr->getNumOperands(); ++OpNum) {
     Value *Operand = Instr->getOperand(OpNum);
     auto it = ReplaceWithConstMap.find(Operand);
     if (it != ReplaceWithConstMap.end()) {
+      assert(!isa<Constant>(Operand) &&
+             "Replacing constants with constants is invalid");
       Instr->setOperand(OpNum, it->second);
       Changed = true;
     }
   }
   return Changed;
 }
 
 /// The given values are known to be equal in every block
bool GVN::processBlock(BasicBlock *BB) {
   // Clearing map before every BB because it can be used only for single BB.
   ReplaceWithConstMap.clear();
   bool ChangedFunction = false;
 
   for (BasicBlock::iterator BI = BB->begin(), BE = BB->end();
        BI != BE;) {
     if (!ReplaceWithConstMap.empty())
       ChangedFunction |= replaceOperandsWithConsts(BI);
-
     ChangedFunction |= processInstruction(BI);
+
     if (InstrsToErase.empty()) {
       ++BI;
       continue;
     }
 
     // If we need some instructions deleted, do it now.
     NumGVNInstr += InstrsToErase.size();
 

test/Transforms/GVN/assume-equal.ll

 define i32 @_Z1ii(i32 %p) {
 entry:
   %cmp = icmp eq i32 %p, 42
   call void @llvm.assume(i1 %cmp)
   
   ; CHECK: br i1 true, label %bb2, label %bb2
   br i1 %cmp, label %bb2, label %bb2
 bb2:
+  call void @llvm.assume(i1 true)
   ; CHECK: br i1 true, label %bb2, label %bb2
   br i1 %cmp, label %bb2, label %bb2
   
   ; CHECK: ret i32 42
   ret i32 %p
 }
 
+; CHECK-LABEL: define i32 @_Z1ij(i32 %p)
+define i32 @_Z1ij(i32 %p) {
+entry:
+  %cmp = icmp eq i32 %p, 42
+  call void @llvm.assume(i1 %cmp)
+  
+  ; CHECK: br i1 true, label %bb2, label %bb2
+  br i1 %cmp, label %bb2, label %bb2
+bb2:
+   ; CHECK-NOT: %cmp2 = 
+  %cmp2 = icmp eq i32 %p, 42
+  ; CHECK-NOT: call void @llvm.assume(
+  call void @llvm.assume(i1 %cmp2)
+  
+  ; CHECK: br i1 true, label %bb2, label %bb2
+  br i1 %cmp, label %bb2, label %bb2
+  
+  ; CHECK: ret i32 42
+  ret i32 %p
+}
+
+; CHECK-LABEL: define i32 @_Z1ik(i32 %p)
+define i32 @_Z1ik(i32 %p) {
+entry:
+  %cmp = icmp eq i32 %p, 42
+  call void @llvm.assume(i1 %cmp)
+  
+  ; CHECK: br i1 true, label %bb2, label %bb3
+  br i1 %cmp, label %bb2, label %bb3
+bb2:
+  ; CHECK-NOT: %cmp3 = 
+  %cmp3 = icmp eq i32 %p, 43
+  ; CHECK: store i8 undef, i8* null
+  call void @llvm.assume(i1 %cmp3)
+  ret i32 15
+bb3:
+  ret i32 17
+}
+
 declare noalias i8* @_Znwm(i64)
 declare void @_ZN1AC1Ev(%struct.A*)
 declare void @llvm.assume(i1)
 declare i32 @_ZN1A3fooEv(%struct.A*)
 declare i32 @_ZN1A3barEv(%struct.A*)