Page MenuHomePhabricator

Fix constant-folding of overflowing arithmetic ops on vectors
ClosedPublic

Authored by LemonBoy on Oct 17 2020, 10:42 AM.

Details

Summary

Feeding vector values to InstCombiner::OptimizeOverflowCheck produces a scalar boolean flag if it proves the overflow check can be eliminated.
This causes InstCombiner::CreateOverflowTuple to crash as it correctly expects a vector of i1 values instead.

Diff Detail

Event Timeline

LemonBoy created this revision.Oct 17 2020, 10:42 AM
LemonBoy requested review of this revision.Oct 17 2020, 10:42 AM
LemonBoy updated this revision to Diff 298846.Oct 17 2020, 12:03 PM

Oh well, it turns out the results are evaluated according to all the elements so it's ok to splat it.

nikic added a subscriber: nikic.Oct 17 2020, 12:07 PM

Doesn't the same issue apply to the isNeutralValue branch?

lebedev.ri added inline comments.Oct 17 2020, 12:13 PM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4607

This seems like wrong granularity.
I think there should be some method that would return an appropriate i1 or vector of i1 type,
which you'd then just pass into ConstantInt::getFalse()/ConstantInt::getTrue().

LemonBoy updated this revision to Diff 298850.Oct 17 2020, 1:30 PM

Fix the neutral value code path too.

Doesn't the same issue apply to the isNeutralValue branch?

Duh, fixed that too.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4607

That's the prettiest way to get a constant i1xN vector I found. I'm not too familiar with the internal LLVM APIs so suggestions are more than welcome.

LemonBoy updated this revision to Diff 299425.Oct 20 2020, 11:20 AM

Cleaned up the true/false building parts.
Added a note about LLVM not detecting if signed multiplication overflow, not much of a problem and can be fixed later.

lebedev.ri accepted this revision.Oct 20 2020, 11:24 AM

Looks good to me, thanks.

This revision is now accepted and ready to land.Oct 20 2020, 11:24 AM
LemonBoy added a subscriber: hans.Oct 20 2020, 1:40 PM

Looks good to me, thanks.

Great, can you commit this on my behalf? I have no commit access.

@hans Is it too early to request this patch to be included in 11.0.1?

Looks good to me, thanks.

Great, can you commit this on my behalf? I have no commit access.

@hans Is it too early to request this patch to be included in 11.0.1?

Please specify the Author: name <e@ma.il> to be used for the commit.

Looks good to me, thanks.

Great, can you commit this on my behalf? I have no commit access.

@hans Is it too early to request this patch to be included in 11.0.1?

Please specify the Author: name <e@ma.il> to be used for the commit.

The name is LemonBoy, you can get the mail from running git log --author=LemonBoy in the LLVM repo.
TIA

LemonBoy retitled this revision from Don't fold overflowing arithmetic ops on vectors to Fix constant-folding of overflowing arithmetic ops on vectors.Sat, Oct 31, 6:45 AM
LemonBoy edited the summary of this revision. (Show Details)
majnemer added inline comments.Sat, Oct 31, 9:27 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4590–4592

I think this can be simplified to CmpInst::makeCmpResultType(LHS->getType())

LemonBoy added inline comments.Sat, Oct 31, 10:15 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4590–4592

Yup, I didn't use that because this is not the result of a comparison despite having the same type as one.

Terribly sorry for that, landed now.
Please just get commit access yourself :)