Page MenuHomePhabricator

Failure to vectorize __builtin_sqrt/__builtin_sqrtf
AbandonedPublic

Authored by avt77 on Dec 9 2016, 7:04 AM.

Details

Summary

This review should resolve Bug 27435 - [X86][SSE] Failure to vectorize builtin_sqrt/builtin_sqrtf. All details you could find here: PR27435.
The problem was found in C-file compiling that's why I added C-test - maybe I'm wrong?
The fix is really tiny: I simply removed the check which should be done in another place if it's needed.

Diff Detail

Event Timeline

avt77 updated this revision to Diff 80896.Dec 9 2016, 7:04 AM
avt77 retitled this revision from to Failure to vectorize __builtin_sqrt/__builtin_sqrtf.
avt77 updated this object.
avt77 added reviewers: RKSimon, spatel, ABataev.
fhahn added a subscriber: fhahn.Dec 9 2016, 3:31 PM

It would be great the have a ll IR test case. You can use the -S -emit-llvm options to generate llvm IR for the test case.

RKSimon edited edge metadata.Dec 10 2016, 2:28 AM
RKSimon added a subscriber: llvm-commits.

It would be great the have a ll IR test case. You can use the -S -emit-llvm options to generate llvm IR for the test case.

In fact we should only have ll tests - depending on clang in the llvm project is a big no-no. Move the .c test over to clang/test/Codegen, checking the generated IR in that file; then have an equivalent test in llvm that tests the codegen for exactly that IR. You can add the clang test without review.

lib/Analysis/ValueTracking.cpp
2529

Do we know the history behind why sqrt was protected by this?

test/CodeGen/X86/lit.local.cfg
7 ↗(On Diff #80896)

c/cpp test files aren't permitted in the llvm project

hfinkel added inline comments.
lib/Analysis/ValueTracking.cpp
2529

The problem, which has come up a lot, is that our sqrt intrinsic is a special case. Unlike the other intrinsics for math functions, it does not exactly mirror the behavior of the corresponding libm function. As noted in the LangRef, "Unlike sqrt in libm, however, llvm.sqrt has undefined behavior for negative numbers other than -0.0." Thus, unless we get to assume no NaNs, it is possible that the intrinsic might have UB in cases where the libm function does not, and as a result, we can't substitute the intrinsic for the libm call.

fhahn added inline comments.Dec 11 2016, 5:42 AM
lib/Analysis/ValueTracking.cpp
2529

I think the checks were added in https://reviews.llvm.org/rL265521 and I think the reasoning behind the check is still sound.

LibFun::sqrt and Intrinsic::sqrt differ in the way they treat negative elements, the intrinsic has undefined behavior for negative numbers (http://llvm.org/docs/LangRef.html#llvm-sqrt-intrinsic).

RKSimon added inline comments.Dec 11 2016, 9:38 AM
lib/Analysis/ValueTracking.cpp
2529

What do you think is the best approach going forward? Is there a way to detect when the target lib func will in fact lower to an instruction (as per SSE sqrtss/sqrtsd) and will handle nan correctly?

Failing that it starting to sound like we should just do a fixup vectorization during x86 lowering of a build_vector of sqrt ops as I suggested in https://llvm.org/bugs/show_bug.cgi?id=27435#c3

hfinkel added inline comments.Dec 12 2016, 1:33 AM
lib/Analysis/ValueTracking.cpp
2529

What do you think is the best approach going forward? Is there a way to detect when the target lib func will in fact lower to an instruction (as per SSE sqrtss/sqrtsd) and will handle nan correctly?

We can't have target-dependent semantics for a target-independent IR-level intrinsic. I'd prefer that we somehow eliminate the IR-level special case here, either by adding a new intrinsic to represent sqrt with the regular semantics or add some argument to the existing intrinsic to indicate that all negative numbers, etc. are supported. A second intrinsic is probably easier.

The fact that we have an intrinsic corresponding to the libm functions we handle except for sqrt, for which this is only true in no-NaNs mode, causes all sorts of inconveniences (and, as in this case, missed opportunities).

avt77 updated this revision to Diff 81934.Dec 19 2016, 3:46 AM
avt77 edited edge metadata.

I restored the check if (ICS->hasNoNaNs()) but for non-vector operations only because vector instructions work correctly with invalid input values. To make it possible I changed the signature of llvm::getIntrinsicForCallSite. Now it knows the required intrinsic target.

RKSimon added inline comments.Jan 6 2017, 6:09 AM
lib/Analysis/ValueTracking.cpp
2529

I don't think we can use an isvector test to determine if its safe to replace a sqrt libcall with a llvm.sqrt intrinsic.

test/CodeGen/X86/vector-sqrt.ll
3

This isn't testing your patch - surely you need a tests in somewhere like llvm\test\Transforms\SLPVectorizer\X86\sqrt.ll testing sqrt/sqrtf libcalls as well as the llvm.sqrt intrinsics? Testing at the IR level not asm codegen.

avt77 added inline comments.Jan 9 2017, 1:35 AM
lib/Analysis/ValueTracking.cpp
2529

Does it mean you insist on a new intrinsic something like here:
if (ICS->hasNoNaNs())

return Intrinsic::sqrt;

else

return Intrinsic:sqrtWithoutNoNaNs();

This new intrinsic should check the real argument and if it's NaNs it should call stdlib otherwise it should call Intrinsic:sqrt. Right?

hfinkel added inline comments.Jan 9 2017, 9:38 AM
lib/Analysis/ValueTracking.cpp
2529

Yes, I think that, if we are going to use this intrinsic canonicalization in this part of the code, then we need a separate representation for the "regular" sqrt as opposed to our special NoNaNs sqrt. Or we should just fix our current sqrt intrinsic (as Eli recently proposed in an RFC). Since we now can add nnan to calls, we don't seem to need the special semantics even to preserve the same representational capability.

ABataev resigned from this revision.Feb 13 2017, 11:41 AM
spatel edited edge metadata.Dec 1 2017, 6:47 AM

Can we abandon this now? PR27435 was closed with D39642 / rL317519. The steps needed to fix the general problem with errno are listed in D28335.

avt77 abandoned this revision.Dec 7 2017, 12:53 AM

As spatel wrote : "PR27435 was closed with D39642 / rL317519. The steps needed to fix the general problem with errno are listed in D28335."