This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Add transforms for `(icmp {u|s}ge/le (xor X, Y), X)`
ClosedPublic

Authored by goldstein.w.n on Feb 22 2023, 5:20 PM.

Details

Summary

If Y is non-zero we can simplify the ge/le -> gt/lt

(X ^ Y_NonZero) u>= X --> (X ^ Y_NonZero) u> X

(X ^ Y_NonZero) u<= X --> (X ^ Y_NonZero) u< X

(X ^ Y_NonZero) s>= X --> (X ^ Y_NonZero) s> X

(X ^ Y_NonZero) s<= X --> (X ^ Y_NonZero) s< X

Diff Detail

Event Timeline

goldstein.w.n created this revision.Feb 22 2023, 5:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2023, 5:20 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
goldstein.w.n requested review of this revision.Feb 22 2023, 5:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2023, 5:20 PM

This is adding a lot of code to a function that's already too big. A helper function will make it easier to read.

This seems like 2 or 3 independent patches? I just looked at the first group of 4 Alive proofs, and those seem fine.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4489

Use isNonStrictPredicate() / getStrictPredicate() ?

This is adding a lot of code to a function that's already too big. A helper function will make it easier to read.

This seems like 2 or 3 independent patches? I just looked at the first group of 4 Alive proofs, and those seem fine.

Sorry just saw this, will do so, likewise for the or patch.

Split into two patch + move to helper

goldstein.w.n retitled this revision from [InstCombine] Add transforms for `(icmp (xor X, Y), X)` to [InstCombine] Add transforms for `(icmp {u|s}ge/le (xor X, Y), X)`.Mar 3 2023, 4:00 PM
goldstein.w.n edited the summary of this revision. (Show Details)
goldstein.w.n marked an inline comment as done.Mar 3 2023, 4:06 PM
goldstein.w.n edited the summary of this revision. (Show Details)

Use getstrictpred

spatel added inline comments.Mar 5 2023, 7:51 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4159–4161

Can we canonicalize the xor as operand 0?
It shouldn't make much difference on this patch, but that would make the next one smaller.

// Normalize xor operand as operand 0.
CmpInst::Predicate Pred = I.getPredicate();
if (match(Op1, m_c_Xor(m_Specific(Op0), m_Value()))) {
  std::swap(Op0, Op1);
  Pred = ICmpInst::getSwappedPredicate(Pred);
}
if (!match(Op0, m_c_Xor(m_Specific(Op1), m_Value(A))))
  return nullptr;
llvm/test/Transforms/InstCombine/icmp-of-xor-x.ll
41

This is testing the commuted xor, so that's good - but it would be better to commute the xor operands in the original test, so we are not relying on some other transform to get to canonical form.

73–74

Similar to above, it would be better if the xor operands are swapped originally.
But we need another instruction to make this test provide coverage for the pattern where the xor is operand 1 of the icmp. Ie, %x needs to be generated by a binop similar to the test just above this one.

goldstein.w.n marked an inline comment as done.Mar 5 2023, 12:43 PM
goldstein.w.n added inline comments.
llvm/test/Transforms/InstCombine/icmp-of-xor-x.ll
41

This is testing the commuted xor, so that's good - but it would be better to commute the xor operands in the original test, so we are not relying on some other transform to get to canonical form.

Sorry not sure I understand what you mean by "in the original test" or what you want changed.

Did update this patch (along with follow / or to swap op0/op1 so op0 is always the xor.

Swap ops to cannonicalize and cleanup logic

spatel added inline comments.Mar 6 2023, 6:45 AM
llvm/test/Transforms/InstCombine/icmp-of-xor-x.ll
41

I posted comments directly in D144607 to hopefully make it clearer.

spatel accepted this revision.Mar 6 2023, 10:57 AM

LGTM

This revision is now accepted and ready to land.Mar 6 2023, 10:57 AM
This revision was landed with ongoing or failed builds.Apr 17 2023, 8:39 PM
This revision was automatically updated to reflect the committed changes.
goldstein.w.n reopened this revision.Apr 18 2023, 1:32 PM

This doesn't appear to have been related to PR62175 so repushing.

This revision is now accepted and ready to land.Apr 18 2023, 1:32 PM
goldstein.w.n closed this revision.Apr 19 2023, 5:32 PM