This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] (rot X, ?) == 0/-1 --> X == 0/-1
ClosedPublic

Authored by Chenbing.Zheng on May 9 2022, 4:33 AM.

Details

Summary

This transform is copied from foldICmpEqIntrinsicWithConstant,
it is safe to re-use undef elts in a vector.
This patch complete it in foldICmpInstWithConstantNotInt.

Diff Detail

Event Timeline

Chenbing.Zheng created this revision.May 9 2022, 4:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 4:33 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Chenbing.Zheng requested review of this revision.May 9 2022, 4:33 AM
spatel added a comment.May 9 2022, 5:42 AM

This approach will not scale well. There are many transforms where we can allow undefs, but we did not bother to do it because there was no immediate motivation.
Also, if we are duplicating code/logic, then we are increasing the risk of bugs that are very hard to test/notice.

I would prefer that we back through the callers to the line where C was captured and allow matching vectors with undefs (m_APIntAllowUndef). Then add a parameters/logic to decide if a vector constant with undef is acceptable.

If that is too hard, then add an alternate code path from the caller for transforms where undef is allowed. Then move the transform under that caller rather than copying code.

This approach will not scale well. There are many transforms where we can allow undefs, but we did not bother to do it because there was no immediate motivation.
Also, if we are duplicating code/logic, then we are increasing the risk of bugs that are very hard to test/notice.

I would prefer that we back through the callers to the line where C was captured and allow matching vectors with undefs (m_APIntAllowUndef). Then add a parameters/logic to decide if a vector constant with undef is acceptable.

If that is too hard, then add an alternate code path from the caller for transforms where undef is allowed. Then move the transform under that caller rather than copying code.

Yer, I agree with you, but there are too many functions involved. I will try to do it. As a first step I will simplify function ‘foldICmpInstWithConstant’ D125457.

RKSimon requested changes to this revision.May 16 2022, 4:11 AM

Not sure if this patch will be abandoned or refactored after D125457

This revision now requires changes to proceed.May 16 2022, 4:11 AM
spatel added inline comments.May 17 2022, 8:44 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
3043–3044

This comment is not needed. The comment above the new function is enough.

3288–3289

Delete this comment. We did not copy the transform (because that would be bad design). We moved it.

The safety of allowing undef is already mentioned in the function-level comment and the function name, so there is no need to repeat it here.

llvm/test/Transforms/InstCombine/icmp-fsh.ll
64

We should add one more test that has "eq zero with undef", so the transform has better coverage.

If there is not a test for a non-zero and non-negative-one, then please add that too. Then we know the transform does not happen with something like "<i5 undef, i5 1>".

adderss comments

Chenbing.Zheng marked 3 inline comments as done.May 17 2022, 7:11 PM
Chenbing.Zheng added inline comments.
llvm/test/Transforms/InstCombine/icmp-fsh.ll
64

I have committed tests, and the negative tests follow here.

spatel accepted this revision.May 18 2022, 6:40 AM

LGTM - if you are planning to enhance more folds to allow undef in vector constants, please test correctness with Alive2. It is true that there are more folds like this, but it is not correct to propagate undef elements in general.
Please wait for @RKSimon for final approval.

RKSimon accepted this revision.May 18 2022, 7:04 AM

LGTM - cheers

This revision is now accepted and ready to land.May 18 2022, 7:04 AM
This revision was landed with ongoing or failed builds.May 18 2022, 8:29 PM
This revision was automatically updated to reflect the committed changes.
Chenbing.Zheng marked an inline comment as done.