Page MenuHomePhabricator

Fix arm_neon.h to be clean under -fno-lax-vector-conversions.
AbandonedPublic

Authored by rsmith on May 8 2019, 11:17 PM.

Details

Summary

arm_neon.h produces a large number of errors under -fno-lax-vector-conversions,
which disables some deeply dangerous implicit conversions and which we really
need to make the default (PR17164).

This patch fixes the immediate problem by removing a bunch of incorrect casts
generated in the header. But it's only the first part of the problem: the other
part is fixing the incorrect expectations in the corresponding CodeGen test
files (in particular, in two ~20K line test files). However, I don't want to
touch that until I'm confident this change is correct.

Diff Detail

Event Timeline

rsmith created this revision.May 8 2019, 11:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2019, 11:17 PM
rsmith updated this revision to Diff 198762.May 8 2019, 11:19 PM

Remove unneeded test change.

SjoerdMeijer accepted this revision.May 9 2019, 1:28 AM

Looks okay to me.

This revision is now accepted and ready to land.May 9 2019, 1:28 AM
rsmith abandoned this revision.Sep 12 2019, 5:32 PM

This patch doesn't work, and the ARM NEON intrinsic header emitter is too opaque and mysterious for me to be able to fix this, and it's not even clear that the generator knows what the actual parameter types of the compiler builtins are. For now I think the best I can do is to leave ARM targets with lax vector conversions enabled, and disable it for all the other targets. :-(

(It's also completely ridiculous that we're generating a 63K LoC (2.1MB) header file for this; our approach to intrinsics headers is unreasonable in the extreme. But I digress.)

Yeah, sorry about that....

Do you perhaps have a test case or error that I can look at? Perhaps I or someone else here can help out a bit here.

Do you perhaps have a test case or error that I can look at? Perhaps I or someone else here can help out a bit here.

You can reproduce the problem with, for example:

echo '#include <arm_neon.h>' | clang -target arm64-linux-gnu -arch +neon -fsyntax-only -x c - -fno-lax-vector-conversions

I've cleaned up all our other intrinsics headers to be clean under at least -flax-vector-conversions=integer. I'll be switching the default for all targets other than ARM NEON to that imminently. (It'll take a bit more work to be able to switch to -fno-lax-vector-conversions by default, but getting there should be our goal.)

I've filed PR43341 for this issue.

Many thanks! I've passed on the message, hopefully we can do something about this.