Page MenuHomePhabricator

[InstCombine] Fold 'check for [no] signed truncation' pattern
ClosedPublic

Authored by lebedev.ri on Jul 13 2018, 2:06 PM.

Details

Summary

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.

The DAGCombine will reverse this transform, see
https://reviews.llvm.org/D49266

This transform is surprisingly frustrating.
This does not deal with non-splat shift amounts, or with undef shift amounts.
I've outlined what i think the solution should be:

// Potential handling of non-splats: for each element:
//  * if both are undef, replace with constant 0.
//    Because (1<<0) is OK and is 1, and ((1<<0)>>1) is also OK and is 0.
//  * if both are not undef, and are different, bailout.
//  * else, only one is undef, then pick the non-undef one.

This is a re-commit, as the original patch, committed in rL337190
was reverted in rL337344 as it broke chromium build:
https://bugs.llvm.org/show_bug.cgi?id=38204 and
https://crbug.com/864832
Proofs that the fixed folds are ok: https://rise4fun.com/Alive/VYM

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jul 13 2018, 2:06 PM
spatel accepted this revision.Jul 16 2018, 8:32 AM

LGTM

This revision is now accepted and ready to land.Jul 16 2018, 8:32 AM

LGTM

Thank you for the review.
I'm not sure about the case with undef's.
Right now, honestly, i only care about the simple scalar case, so i'll just leave that unimplemented for now..

This revision was automatically updated to reflect the committed changes.
lebedev.ri reopened this revision.Jul 17 2018, 11:19 PM
lebedev.ri added a subscriber: inglorion.

Was reverted in rL337344 by @inglorion:

Revert "[InstCombine] Fold 'check for [no] signed truncation' pattern"

This reverts r337190 (and a few follow-up commits), which caused the
Chromium build to fail. See
https://bugs.llvm.org/show_bug.cgi?id=38204 and
https://crbug.com/864832

Not sure what is the problem yet.

This revision is now accepted and ready to land.Jul 17 2018, 11:19 PM
lebedev.ri edited the summary of this revision. (Show Details)

Update the diff in the differential with the straight un-revert for now.
Will look into the problem next.

Rebase on top of more tests, that show miscompile.

Operate on APInt's directly, fixes PR38204.

Once this handled non-splats, this will have to not operate on these APInt's,
but operate on ConstantInt vectors directly anyway.

lebedev.ri edited the summary of this revision. (Show Details)Jul 18 2018, 3:30 AM
lebedev.ri edited the summary of this revision. (Show Details)Jul 18 2018, 3:44 AM
This revision was automatically updated to reflect the committed changes.