Page MenuHomePhabricator

Don't fold overflowing arithmetic ops on vectors
AcceptedPublic

Authored by LemonBoy on Sat, Oct 17, 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.

Skip the overflow check elision for the time being as the code produces incorrect results for vector types anyway.

Diff Detail

Event Timeline

LemonBoy created this revision.Sat, Oct 17, 10:42 AM
LemonBoy requested review of this revision.Sat, Oct 17, 10:42 AM
LemonBoy updated this revision to Diff 298846.Sat, Oct 17, 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.Sat, Oct 17, 12:07 PM

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

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

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.Sat, Oct 17, 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
4595

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.Tue, Oct 20, 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.Tue, Oct 20, 11:24 AM

Looks good to me, thanks.

This revision is now accepted and ready to land.Tue, Oct 20, 11:24 AM
LemonBoy added a subscriber: hans.Tue, Oct 20, 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?