This is an archive of the discontinued LLVM Phabricator instance.

[TTI] Check that lowered type is floating point before calling isFabsFree
ClosedPublic

Authored by sbc100 on Oct 4 2018, 11:55 AM.

Details

Summary

Fixes a crash in SLP vectoriser under soft-fp.

In the case of soft-fp (e.g. fp128 under wasm) the result of
getTypeLegalizationCost() can be an integer type even if the input is
floating point (See LegalizeTypeAction::TypeSoftenFloat).

Before calling isFabsFree() (which asserts if given a non-fp
type) we need to check that that result is fp. This is safe since in
fabs is certainly not free in the soft-fp case.

Fixes PR39168

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Oct 4 2018, 11:55 AM
sbc100 retitled this revision from [SLPVectorizer] Check that lowered type is floating point before calling isFabsFree to [TTI] Check that lowered type is floating point before calling isFabsFree.Oct 4 2018, 11:57 AM
sbc100 edited the summary of this revision. (Show Details)
sbc100 added reviewers: arsenm, dschuff.

I can trigger the same issue by using __float128 types and --target=i686-linux (where fp128 is emulated in software).

sbc100 updated this revision to Diff 168382.Oct 4 2018, 2:35 PM
  • add test
sbc100 added a comment.Oct 4 2018, 2:37 PM

This is the smallest test I could create. I would love it to be smaller but it seems hard to tickle this particular path.

efriedma added inline comments.Oct 4 2018, 3:29 PM
test/Transforms/SLPVectorizer/X86/software-float.ll
6 ↗(On Diff #168382)

Probably should call FileCheck to make sure the code is actually vectorized the way you expect.

sbc100 added inline comments.Oct 4 2018, 5:36 PM
test/Transforms/SLPVectorizer/X86/software-float.ll
6 ↗(On Diff #168382)

I actually don't know if it successfully vectorizing. I think we only really care that this doesn't crash here don't we?

Right but the point is that we don't currently have a way to ensure that the code is doing its job; if the SLP vectorizer stops doing its thing and calling the code that was asserting, then this test won't be testing anything.

sbc100 added a comment.Oct 4 2018, 5:51 PM

I'm not sure it actually does the vectorization in the end. The bit-code that some out is basically identical.

I get the following stuff in -debug-only=SLP:

SLP: Analyzing blocks in __multc3.
SLP: Trying to vectorize starting at PHIs (1)
SLP: Trying to vectorize a list of length = 2.
SLP: Trying to vectorize starting at PHIs (2)
SLP: Trying to vectorize a list of length = 2.
SLP: Analyzing 2 operations 
SLP: We are able to schedule this bundle.
...
...
SLP: Calculating cost for tree of size 4.
SLP: Adding cost 0 for bundle that starts with   %cmpinf = fcmp oeq fp128 %0, 0xL00000000000000007FFF000000000000.
SLP: Call cost 0 (16-16) for   %0 = tail call fp128 @llvm.fabs.f128(fp128 %a)
SLP: Adding cost 0 for bundle that starts with   %0 = tail call fp128 @llvm.fabs.f128(fp128 %a).
SLP: Adding cost 0 for bundle that starts with fp128 %a.
SLP: Adding cost 0 for bundle that starts with fp128 0xL00000000000000007FFF000000000000.
SLP: #LV: 1 , Looking at   %0 = tail call fp128 @llvm.fabs.f128(fp128 %a)
SLP: Spill Cost = 0.
SLP: Extract Cost = 2.
SLP: Total Cost = 2.
SLP: Trying to vectorize a list of length = 2.
SLP: Trying to vectorize a list of length = 2.

I don't really know how to interpret the debug output, but I don't see any changes made to the bit code.

arsenm added inline comments.Oct 4 2018, 7:10 PM
test/Transforms/SLPVectorizer/X86/software-float.ll
6 ↗(On Diff #168382)

No, tests that don't check anything are considered bad form. You can also make this testcase a lot smaller. You may want/need to set the cost threshold to a negative value to force it

sbc100 updated this revision to Diff 168536.Oct 5 2018, 2:42 PM
  • add filecheck and minimize test
sbc100 added a comment.Oct 5 2018, 2:43 PM

Thanks for the tip, setting a negative threshold helped, and I was able to reduce the test case too.

dschuff accepted this revision.Oct 9 2018, 11:04 AM

Test looks much nicer now, LGTM

This revision is now accepted and ready to land.Oct 9 2018, 11:04 AM
This revision was automatically updated to reflect the committed changes.