Page MenuHomePhabricator

[InstCombine] enhance icmp with sub folds
ClosedPublic

Authored by Allen on Sat, Mar 11, 6:30 AM.

Details

Summary

The new IR with And removes a use of the input variable, which is better for analysis.

Diff Detail

Event Timeline

Allen created this revision.Sat, Mar 11, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Mar 11, 6:30 AM
Allen requested review of this revision.Sat, Mar 11, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Mar 11, 6:30 AM
Allen edited the summary of this revision. (Show Details)Thu, Mar 16, 4:26 AM
goldstein.w.n added inline comments.
llvm/test/Transforms/InstCombine/icmp-sub.ll
564

Can you split the tests into two commits so we can see the diff from the patch?

The new IR with and removes a use of the input variable, which is better for analysis.

"and" -> And (otherwise it looks like the conjugation, not the operator.

Allen updated this revision to Diff 505971.Thu, Mar 16, 7:37 PM
Allen marked an inline comment as done.
Allen edited the summary of this revision. (Show Details)

1、Precommit the test
2、Add m_OneUse

Allen added a comment.Thu, Mar 16, 7:38 PM

The new IR with and removes a use of the input variable, which is better for analysis.

"and" -> And (otherwise it looks like the conjugation, not the operator.

Done, thanks

llvm/test/Transforms/InstCombine/icmp-sub.ll
564

Done, I Precommit the test with D146268

nikic added a subscriber: nikic.Sun, Mar 19, 2:06 AM
nikic added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4096

Use ICmpInst::isEquality().

4098

Please add a vector test, this is probably going to crash. (Can use getScalarSizeInBits instead.)

4103

I.getPrediate() -> Pred.

llvm/test/Transforms/InstCombine/icmp-sub.ll
590

Missing commuted test case (negation on RHS), multi-use test case and negative tests (e.g. non-equality predicate).

Allen updated this revision to Diff 506465.Sun, Mar 19, 10:18 PM

address commet
1、Add more cases (also update on D146268)
2、update with getScalarSizeInBits and add related vector case
3、use ICmpInst::isEquality(Pred) to replace Pred == ICmpInst::ICMP_EQ
4、I.getPredicate() --> Pred

Allen marked 4 inline comments as done.Tue, Mar 21, 4:07 AM
nikic added inline comments.Tue, Mar 21, 7:55 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4096

isEquality() means "EQ" or "NE". You don't need to check for ICMP_NE separately.

llvm/test/Transforms/InstCombine/icmp-sub.ll
596

These end up not actually commuted due to complexity-based canonicalization. Grep for thwart complexity-based canonicalization for how to avoid it.

Allen updated this revision to Diff 507259.Wed, Mar 22, 12:52 AM

Address comment
1、delete the repeat ICMP_NE check, as isEquality() already include it
2、use "thwart complexity-based canonicalization" for commuted case

Allen marked 2 inline comments as done.Wed, Mar 22, 12:53 AM
nikic accepted this revision.Wed, Mar 22, 1:03 AM

LGTM

This revision is now accepted and ready to land.Wed, Mar 22, 1:03 AM
This revision was landed with ongoing or failed builds.Wed, Mar 22, 4:57 AM
This revision was automatically updated to reflect the committed changes.