Page MenuHomePhabricator

Don't fold overflowing arithmetic ops on vectors

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



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

Unit TestsFailed

270 mswindows > lld.ELF/invalid::symtab-sh-info.s
Script: -- : 'RUN: at line 4'; c:\ws\w1\llvm-project\premerge-checks\build\bin\yaml2obj.exe --docnum=1 C:\ws\w1\llvm-project\premerge-checks\lld\test\ELF\invalid\symtab-sh-info.s -o C:\ws\w1\llvm-project\premerge-checks\build\tools\lld\test\ELF\invalid\Output\symtab-sh-info.s.tmp.o

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

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.


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?