This is an archive of the discontinued LLVM Phabricator instance.

[EarlyCSE] Remove guards with conditions known to be true
ClosedPublic

Authored by mkazantsev on Apr 25 2017, 1:03 AM.

Details

Summary

If a condition is calculated only once, and there are multiple guards on this condition, we should be able
to remove all guards dominated by the first of them. This patch allows EarlyCSE to try to find the condition
of a guard among the known values, and if it is true, remove the guard. Otherwise we keep the guard and
mark its condition as 'true' for future consideration.

Diff Detail

Event Timeline

mkazantsev created this revision.Apr 25 2017, 1:03 AM
reames requested changes to this revision.Apr 26 2017, 6:33 PM
reames added inline comments.
lib/Transforms/Scalar/EarlyCSE.cpp
661

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);

}

664

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.

test/Transforms/EarlyCSE/guards.ll
6

minor: submit a separate patch without further review for the test name changes to simplify the diff.

This revision now requires changes to proceed.Apr 26 2017, 6:33 PM
reames added inline comments.Apr 26 2017, 6:35 PM
test/Transforms/EarlyCSE/guards.ll
270

Not in EarlyCSE (which isn't allowed to modify CFG). We can fold the condition to false at use though. That should be a separate patch.

mkazantsev edited edge metadata.
mkazantsev marked 4 inline comments as done.

Addressed the comments.

test/Transforms/EarlyCSE/guards.ll
6

Actually I've looked into other tests for EarlyCSE and decided to keep the naming as is for consistency.

sanjoy added inline comments.Apr 27 2017, 5:51 PM
lib/Transforms/Scalar/EarlyCSE.cpp
666

You should be able to do if (match(KnownCond, m_One())) to make it a bit more concise, but what you have now is fine too.

reames accepted this revision.Apr 27 2017, 6:37 PM

LGTM

This revision is now accepted and ready to land.Apr 27 2017, 6:37 PM
This revision was automatically updated to reflect the committed changes.