Page MenuHomePhabricator

[X86][AArch64][DAGCombine] Unfold 'check for [no] signed truncation' pattern
ClosedPublic

Authored by lebedev.ri on Jul 12 2018, 1:53 PM.

Details

Summary

Do we want to check for uses here?


PR38149

As discussed in https://reviews.llvm.org/D49179#1158957 and later,
the IR for 'check for [no] signed truncation' pattern can be improved:
https://rise4fun.com/Alive/gBf
^ that pattern will be produced by Implicit Integer Truncation sanitizer,
https://reviews.llvm.org/D48958 https://bugs.llvm.org/show_bug.cgi?id=21530
in signed case, therefore it is probably a good idea to improve it.

But the IR-optimal patter does not lower efficiently, so we want to undo it..

This handles the simple pattern.
There is a second pattern with predicate and constants inverted.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jul 12 2018, 1:53 PM

Actually run $ ninja check-llvm this time, two tests regressed..

lebedev.ri added inline comments.Jul 12 2018, 2:26 PM
test/CodeGen/X86/peep-test-4.ll
133 ↗(On Diff #155268)

i128, Ok, so we want a TLI hook.
(No, don't think isTypeLegal() will work, it regresses some folds here.)

test/CodeGen/X86/setcc-logic.ll
425–438 ↗(On Diff #155268)

I'm not sure what is going on here yet..

Address both test regressions with one fix - new TargetLowering hook.
FIXME: is there something preexising that i could use? I did look, and didn't find any..

lebedev.ri marked 2 inline comments as done.Jul 13 2018, 12:57 AM
lebedev.ri edited the summary of this revision. (Show Details)

Added all the sane negative tests.
Other than the one-use question i think this is ready for the review.

spatel accepted this revision.Jul 15 2018, 9:51 AM

Just a few nits - otherwise, LGTM.

lib/CodeGen/SelectionDAG/TargetLowering.cpp
1853–1855 ↗(On Diff #155411)

Would be better to explain what "signed truncation" means right here...or just remove and let the formulas in the later comments speak for themselves?

lowers badly -> may not lower optimally

1860 ↗(On Diff #155411)

typo: natual

lib/Target/X86/X86ISelLowering.cpp
4801–4802 ↗(On Diff #155411)

Any reason this is in the header file for AArch but in the cpp file for x86?

This revision is now accepted and ready to land.Jul 15 2018, 9:51 AM

Just a few nits - otherwise, LGTM.

Thank you for the review!

lib/Target/X86/X86ISelLowering.cpp
4801–4802 ↗(On Diff #155411)

That seems to be the way the other hooks are defined - in source for X86, in header for aarch64.
What is the preferred way? In headers?

This comment was removed by lebedev.ri.
spatel added inline comments.Jul 15 2018, 10:07 AM
lib/Target/X86/X86ISelLowering.cpp
4801–4802 ↗(On Diff #155411)

Hmm...it's a matter of taste where we draw the "too-complex-for-a-header" line. But in this case, I'd prefer that we disregard the target pref and make identical overrides have the same form, so it's clear that there's no difference between these targets. Go with the header version since that's smaller?

lebedev.ri added inline comments.Jul 15 2018, 10:09 AM
lib/Target/X86/X86ISelLowering.cpp
4801–4802 ↗(On Diff #155411)

OK.

This revision was automatically updated to reflect the committed changes.