Page MenuHomePhabricator

[TargetLowering] Fix undef vector element issue with true/false result handling
ClosedPublic

Authored by RKSimon on Oct 27 2016, 6:12 AM.

Details

Summary

Fixed an issue with vector usage of TargetLowering::isConstTrueVal / TargetLowering::isConstFalseVal boolean result matching.

The comment says that we shouldn't handle constant splat vectors with undef elements. But the the actual code was returning false if the build vector contained no undef elements....

This current iteration of the patch flips the test so that it matches the comment and only accepts build vectors with no undefs. But since this has been like this since 2014, should we take it as a sign that undefs elements are not an issue and I should update the patch to not care about undefs at all?

The change has also unearthed a couple of missed opportunities in AVX512 comparison code that will need to be addressed.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 76011.Oct 27 2016, 6:12 AM
RKSimon retitled this revision from to [TargetLowering] Fix undef vector element issue with true/false result handling.
RKSimon updated this object.
RKSimon added reviewers: chandlerc, hfinkel, delena, spatel.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: llvm-commits.
hfinkel edited edge metadata.Oct 27 2016, 11:49 AM

My inclination is to leave the code as is, and update the comment to match. If we have all undefs, then we return false (presumably, we have better ways to fold an all-undef operand), but a few undefs are fine.

On the other hand, do we generally produce better code if we don't handle undefs at all?

My inclination is to leave the code as is, and update the comment to match. If we have all undefs, then we return false (presumably, we have better ways to fold an all-undef operand), but a few undefs are fine.

OK - I'll update the patch to completely ignore the UndefElements arg - its not necessary (getConstantSplatNode returns NULL if its a build vector of all undefs).

On the other hand, do we generally produce better code if we don't handle undefs at all?

You can see examples of the codegen delta in the patch (the v8i1-masks.ll cases are similar to the real world code that led me to this patch in the first place). The AVX512 vpcmpgtd() -> knotw(vpcmpled()) is a regression, but minimal compared to the gains elsewhere and should be easy to fix later on.

RKSimon updated this revision to Diff 76080.Oct 27 2016, 12:09 PM
RKSimon edited edge metadata.

Updated to not care at about some (but not all!) undefs in a splatted build vector constant.

delena accepted this revision.Nov 1 2016, 5:43 AM
delena edited edge metadata.

I agree that undef elements may be ignored in constant splat.

This revision is now accepted and ready to land.Nov 1 2016, 5:43 AM
This revision was automatically updated to reflect the committed changes.