[PATCH] D32482: [EarlyCSE] Mark the condition of assume intrinsic as true

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:638
     // they're marked as such to ensure preservation of control dependencies),
-    // and this pass will not disturb any of the assumption's control
-    // dependencies.
+    // and this pass will not bother with its removal. However, we should mark
+    // its condition as true for all dominated blocks.
----------------
sanjoy wrote:
> I think you can do:
> 
> ```
> Instruction *CondI;
> if (match(Inst, m_Intrinsic<Intrinsic::assume>(m_Instruction(CondI))) &&
>     SimpleValue::canHandle(CondI))
>   AvailableValues.insert(CondI, ConstantInt::getTrue(BB->getContext()));
> ```
> 
Can you please also just add a test that llvm.assume(false/true) is skipped?



================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:645
+          AvailableValues.insert(CondI, ConstantInt::getTrue(BB->getContext()));
+      }
       DEBUG(dbgs() << "EarlyCSE skipping assumption: " << *Inst << '\n');
----------------
You aren't skipping it, you should say that :)


https://reviews.llvm.org/D32482