This is an archive of the discontinued LLVM Phabricator instance.

LoopUnswitch - Simplify based on known not to a be constant.
ClosedPublic

Authored by trentxintong on Jan 20 2017, 1:36 PM.

Details

Summary

In case we do not know what the condition is in an unswitched loop, but we know its definitely NOT a known constant. We can perform simplifcations based on this information. More specifically we can simplify comparison instructions with the cond...

Event Timeline

trentxintong created this revision.Jan 20 2017, 1:36 PM
trentxintong retitled this revision from In case we do not know what the condition is in an unswitched loop, but we know its definitely NOT a known constant. We can perform simplifcations based on this information. More specifically we can simplify comparison instructions with the cond... to LoopUnswitch - Simplify based on known not to a be constant..Jan 22 2017, 4:44 PM
trentxintong edited the summary of this revision. (Show Details)
trentxintong added a reviewer: efriedma.

Update comments and rename function.

efriedma added inline comments.Feb 21 2017, 11:41 AM
lib/Transforms/Scalar/LoopUnswitch.cpp
1394

Missing check that Val is a ConstantInt.

1396

Maybe use isTrueWhenEqual here, so you can drop the isEquality check?

1396

Err, nevermind, that doesn't work, please ignore.

trentxintong added inline comments.Feb 21 2017, 11:47 AM
lib/Transforms/Scalar/LoopUnswitch.cpp
1394

Val is going to be a ConstantInt, as its used by ICmpInst, I will add an assert though =).

efriedma added inline comments.Feb 21 2017, 12:07 PM
lib/Transforms/Scalar/LoopUnswitch.cpp
1394

It will be a Constant of integer type... but that's not the same thing as a ConstantInt. (For example ptrtoint i32* @g to i64. is a Constant of integer type.)

trentxintong added inline comments.Feb 21 2017, 12:46 PM
lib/Transforms/Scalar/LoopUnswitch.cpp
1394

I see. Thanks for pointing this out to me. But does Val have to be a constantInt here. What we know here is that the Cond != Val and we are trying to simplify a icmpinst, we are not getting the actual integer value of this constant.

Even though I think the Val is always going to be the constant int in the current state of pass.

efriedma added inline comments.Feb 21 2017, 5:42 PM
lib/Transforms/Scalar/LoopUnswitch.cpp
1394

Err, no, you're right... this particular transform works out even if Val isn't a ConstantInt, because you're checking for equality.

Sorry, I feel like I'm completely screwing up this review; I'll take another look tomorrow, and hopefully it's clearer then.

efriedma added inline comments.Feb 22 2017, 4:02 PM
lib/Transforms/Scalar/LoopUnswitch.cpp
242

The name "Cond" is kind of confusing... it's the condition of some switch somewhere, but that isn't particularly relevant here. Maybe "Invariant" is a better name, since that's the important property?

Also, the name of the function isn't great... maybe rename it SimplifyInstructionWithNotEqual?

1389

This comment is a bit difficult to parse. Maybe just "icmp eq cond, val -> false"?

1406

Redundant comment.

Address efriedma's comments.

Address efriedma's comments.

efriedma accepted this revision.Feb 23 2017, 3:11 PM

LGTM.

This revision is now accepted and ready to land.Feb 23 2017, 3:11 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the review @efriedma !

There was an iterator invalidation bug here. I committed 296069 (https://reviews.llvm.org/rL296069) to fix the problem. Post-commit reviews welcome.