This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Remove requirement on `trunc` in `slt/sgt` case in `foldSelectICmpAndOr`
ClosedPublic

Authored by goldstein.w.n on Apr 14 2023, 11:12 PM.

Details

Summary

AFAICT, the trunc is not needed for correctness/performance and just
blocks what should be handlable cases.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Apr 14 2023, 11:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2023, 11:12 PM
goldstein.w.n requested review of this revision.Apr 14 2023, 11:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2023, 11:12 PM
nikic added inline comments.Apr 19 2023, 1:06 PM
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
676

Unless I'm missing something, we're missing an assignment to the V variable in this branch now? Also, do we still need the NeedAnd?

We should really just be using decomposeBitTestICmp() and a pow2 check I think, but this code is doing some very weird things I don't fully get.

goldstein.w.n added inline comments.Apr 19 2023, 1:25 PM
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
676

Unless I'm missing something, we're missing an assignment to the V variable in this branch now?

Good catch not sure how that didn't get caught in the tests.

Also, do we still need the NeedAnd?

Yes. x >= 0 == x & SignBit == 0 but we don't have the actual x & SignBit expression.

We should really just be using decomposeBitTestICmp() and a pow2 check I think, but this code is doing some very weird things I don't fully get.

I think that works. Will make it a follow up patch to this.

goldstein.w.n marked an inline comment as done.Apr 19 2023, 1:44 PM
goldstein.w.n added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
676

Unless I'm missing something, we're missing an assignment to the V variable in this branch now? Also, do we still need the NeedAnd?

Done.

We should really just be using decomposeBitTestICmp() and a pow2 check I think, but this code is doing some very weird things I don't fully get.

See: D148744

nikic added inline comments.Apr 23 2023, 1:17 AM
llvm/test/Transforms/InstCombine/select-with-bitwise-ops.ll
772

Can you add a multi-use variant of this test? I think the reason why this code is checking for the trunc is that it replaces a one-use trunc with an and, which preserves the number of uses. If we drop the check, we'll have to take NeedsAnd into account when counting uses.

Add NeedAnds to use profitability check

goldstein.w.n marked an inline comment as done.Jun 27 2023, 2:48 PM
goldstein.w.n added inline comments.
llvm/test/Transforms/InstCombine/select-with-bitwise-ops.ll
772

Good point. Added NeedAnd to muiltiuse check.

nikic accepted this revision.Jun 29 2023, 7:12 AM

LGTM

This revision is now accepted and ready to land.Jun 29 2023, 7:12 AM
goldstein.w.n marked an inline comment as done.

Rebase

This revision was landed with ongoing or failed builds.Aug 16 2023, 8:43 PM
This revision was automatically updated to reflect the committed changes.