Page MenuHomePhabricator

[InstCombine] Teach foldOrOfICmps to allow icmp eq MIN_INT/MAX to be part of a range comparision. Similar for foldAndOfICmps
ClosedPublic

Authored by craig.topper on Jul 19 2019, 2:00 PM.

Details

Summary

We can treat icmp eq X, MIN_UINT as icmp ule X, MIN_UINT and allow
it to merge with icmp ugt X, C. Similar for the other constants.

We can do simliar for icmp ne X, (U)INT_MIN/MAX in foldAndOfICmps. And we already handled UINT_MIN there.

Fixes PR42691.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Jul 19 2019, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2019, 2:00 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Rebase with the tests pre-commited

Harbormaster completed remote builds in B35387: Diff 210894.
craig.topper retitled this revision from [InstCombine] Add test cases for PR42691. NFC to [InstCombine] Teach foldOrOfICmps to allow icmp eq MIN_INT/MAX to be part of a range comparision..Jul 19 2019, 2:09 PM
craig.topper edited the summary of this revision. (Show Details)

Rebase after making one of the tests better

nikic added inline comments.Jul 20 2019, 2:37 AM
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2262 ↗(On Diff #210895)

ULE -> SLE

llvm/test/Transforms/InstCombine/and-or-icmps.ll
316 ↗(On Diff #210895)

This seems like a weird choice that the range check code is making. We could also generate

%x.off = add i32 %x, 1
%c = icmp ult i32 %x.off, -2147483646

with smaller constants.

Fix copy/paste mistake in comment

Also update foldAndOfICmps to handle not equal (U)INT_MIN/MAX

craig.topper retitled this revision from [InstCombine] Teach foldOrOfICmps to allow icmp eq MIN_INT/MAX to be part of a range comparision. to [InstCombine] Teach foldOrOfICmps to allow icmp eq MIN_INT/MAX to be part of a range comparision. Similar for foldAndOfICmps.Jul 21 2019, 8:30 PM
craig.topper edited the summary of this revision. (Show Details)
craig.topper marked an inline comment as done.Jul 23 2019, 10:01 PM
craig.topper added inline comments.
llvm/test/Transforms/InstCombine/and-or-icmps.ll
316 ↗(On Diff #210895)

Is this something this patch should address?

lebedev.ri requested changes to this revision.Jul 24 2019, 12:28 AM
lebedev.ri added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1208–1211 ↗(On Diff #211017)

Ok, https://rise4fun.com/Alive/VUp
Comment does not specify the precondition that C != INT_MIN
I'm guessing that case is handled elsewhere?

1226–1229 ↗(On Diff #211017)

Update comment please: X u> 13 & X != UINT_MAX -> (X-(C+1)) u< UINT_MAX-(C+1)
https://rise4fun.com/Alive/qt2, ok
Comment does not specify the precondition that C != UINT_MAX
I'm guessing that case is handled elsewhere?

1244–1247 ↗(On Diff #211017)

Fix comment please: X s> 13 & X != INT_MAX -> (X-(C+1))) u< INT_MAX-(C+1)
https://rise4fun.com/Alive/hZlO, ok
Comment does not specify the precondition that C != INT_MAX
I'm guessing that case is handled elsewhere?

2269–2309 ↗(On Diff #211017)

Can you please add actual formulas here as comments?

llvm/test/Transforms/InstCombine/and-or-icmps.ll
316 ↗(On Diff #210895)

Is this something this patch should address?

I'm going to go with no. That is a separate fold.

This revision now requires changes to proceed.Jul 24 2019, 12:28 AM

Clean up comments in the new code. And a few minor fixes to the comments in the existing code. Probably should rewrite them to use C instead of specific numbers, but for now I just fixed them to not be completely incorrect about what insertRangeTest does.

craig.topper marked an inline comment as done.Jul 24 2019, 12:04 PM
craig.topper added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1226–1229 ↗(On Diff #211017)

There's an earlier assert above this switch that says LHSC != RHSC which would ensure C can't be UINT_MAX.

lebedev.ri accepted this revision.Jul 24 2019, 12:22 PM

Ok, LGTM, thank you.

llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2269–2272 ↗(On Diff #211579)
2276–2279 ↗(On Diff #211579)
2289–2292 ↗(On Diff #211579)
2304–2308 ↗(On Diff #211579)
llvm/test/Transforms/InstCombine/and-or-icmps.ll
316 ↗(On Diff #210895)

This might warrant filing an issue to track though.

This revision is now accepted and ready to land.Jul 24 2019, 12:22 PM
This revision was automatically updated to reflect the committed changes.