Page MenuHomePhabricator

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

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



Do we want to check for uses here?


As discussed in and later,
the IR for 'check for [no] signed truncation' pattern can be improved:
^ that pattern will be produced by Implicit Integer Truncation sanitizer,
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


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
133 ↗(On Diff #155268)

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

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.

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

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!

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
4801–4802 ↗(On Diff #155411)'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
4801–4802 ↗(On Diff #155411)


This revision was automatically updated to reflect the committed changes.