[PATCH] D32476: [EarlyCSE] Remove guards with conditions known to be true

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:661
               dyn_cast<Instruction>(cast<CallInst>(Inst)->getArgOperand(0))) {
+        // Do we already know the actual value of this condition?
+        if (auto *KnownCond = AvailableValues.lookup(CondI)) {
----------------
I think this would be a bit clearer if put under the canHandle branch.  i.e.
if (canHandle(CondI)) {
  if (auto *KnownCond = ...) {
    ...
  }
  AV.insert(CondI, True);
}


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:664
+          // Is the condition known to be true?
+          if (KnownCond == ConstantInt::getTrue(BB->getContext())) {
+            DEBUG(dbgs() << "EarlyCSE removing guard: " << *Inst << '\n');
----------------
Use isa<ConstantInt>(KnownCond) && cast<ConstantInt>(KnownCond)->isOneValue();

This also might be a case where dyn_cast_or_null would be useful.
auto *KnownCond = ...
auto *KnownCondConst = dyn_cast_or_null...
if (KnownCondConst && KnownCondConst->isOneValue())

Finally, if we've inferred the value to constant false, we should replace the operand with the constant.  Don't bother going beyond that for the moment (i.e. exploiting the unreached code), but there's no reason not to put in the constant.


================
Comment at: test/Transforms/EarlyCSE/guards.ll:6
 
-define i32 @test0(i32* %ptr, i1 %cond) {
+define i32 @test_00(i32* %ptr, i1 %cond) {
 ; We can do store to load forwarding over a guard, since it does not
----------------
minor: submit a separate patch without further review for the test name changes to simplify the diff.


https://reviews.llvm.org/D32476