Page MenuHomePhabricator

[InstCombine] allow multi-use values in canEvaluate* if all uses are in 1 inst
ClosedPublic

Authored by spatel on Jan 31 2018, 8:03 AM.

Details

Summary

This is the enhancement suggested in D42536 to fix a shortcoming in regular InstCombine's canEvaluate* functionality.
When we have multiple uses of a value, but they're all in one instruction, we can allow that expression to be narrowed or widened for the same cost as a single-use value.

AFAICT, this can only matter for multiply: sub/and/or/xor/select would be simplified away if the operands are the same value; add becomes shl; shifts with a variable shift amount aren't handled.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Jan 31 2018, 8:03 AM
aaboud accepted this revision.Feb 1 2018, 6:38 AM

LGTM.
Thanks for taking care of this.

This revision is now accepted and ready to land.Feb 1 2018, 6:38 AM
This revision was automatically updated to reflect the committed changes.
aaboud added inline comments.Feb 5 2018, 7:39 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineCasts.cpp
189

Sorry, I did not notice this before.
But should not we do the same check also for the "Select" and "PHI" below?
Could that be related to the infinite loop caused by this patch?

spatel added inline comments.Feb 5 2018, 8:06 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineCasts.cpp
189

Yes, certainly for the select. I'm not sure about PHI though. A PHI with identical operands would be an invalid instruction?
'PHINode should have one entry for each predecessor of its parent basic block!'

spatel added inline comments.Feb 5 2018, 9:23 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineCasts.cpp
189

I limited the multi-use evaluation to binops with rL324252 . That should disallow select and PHI, so hopefully closes the infinite looping potential. Still trying to come up with a test case.