LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 30792 - [AArch64] -mgeneral-regs-only inconsistent with gcc
Summary: [AArch64] -mgeneral-regs-only inconsistent with gcc
Status: NEW
Alias: None
Product: new-bugs
Classification: Unclassified
Component: new bugs (show other bugs)
Version: trunk
Hardware: PC Linux
: P normal
Assignee: George Burgess
URL:
Keywords:
Depends on:
Blocks: 4068
  Show dependency tree
 
Reported: 2016-10-25 17:28 PDT by Pirama Arumuga Nainar
Modified: 2021-01-10 09:40 PST (History)
14 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Pirama Arumuga Nainar 2016-10-25 17:28:41 PDT
The AArch64 backend runs into a fatal error with the -mgeneral-regs-only when inline assembly refers to a SIMD register.

A reduced test-case:

$ cat > clobber.c << EOF
void dummy() {
    __asm__ volatile ("" ::: "v0");
}
EOF

$ clang -c clobber.c -target aarch64-linux-gnu -mgeneral-regs-only 
fatal error: error in backend: Do not know how to split the result of this
      operator!

The code is compiled by gcc, though.  Seems like gcc is lenient with respect to this flag.  From https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html:
-mgeneral-regs-only
Generate code which uses only the general-purpose registers. This will prevent the compiler from using floating-point and Advanced SIMD registers but will not impose any restrictions on the assembler. 

From http://clang.llvm.org/docs/UsersManual.html:
-mgeneral-regs-only
Generate code which only uses the general purpose registers.
This option restricts the generated code to use general registers only. This only applies to the AArch64 architecture.

There are two possible actions here:
1. Match gcc and allow inline assembly to have SIMD registers.  This may be hard to do, considering that '-mgeneral-regs-only' just passes '-targe
t-feature -fp-armv8 -target-feature -crypto -target-feature -neon' to the driver.

2. Make the driver not crash, and issue an error instead.

This usage seems prevalent in the kernel, which uses -mgeneral-regs-only to avoid saving and restoring the userspace FPSIMD context on every syscall, but hard-codes FPSIMD or crypto instructions in the handful of places they're useful.
Comment 1 Silviu Baranga 2016-11-09 05:04:56 PST
Hi Pirama,

I'm trying to figure out what the best action would be for this. Would option 2) work for the main use case (the kernel)?

As you've noted, matching the gcc behaviour would require some effort (although AFAICT it should be straight-forward to do).

Thanks,
Silviu
Comment 2 Pirama Arumuga Nainar 2016-11-09 13:18:56 PST
Hi Silviu, based on my understanding, yes, it'd be really useful from a performance perspective to match gcc, especially if it is straightforward.

https://android.googlesource.com/kernel/tegra/+/android-tegra-3.10/arch/arm64/crypto/aes-ce-cipher.c is one example of such a file.  Disabling -mgeneral-regs-only for just these files would still necessitate saving of FPSIMD context.  Extracting the inline assembly out to separate files would mean relying on lto (which I am not sure is functional for kernel builds yet.
Comment 3 Silviu Baranga 2016-11-15 10:40:14 PST
Hi Pirama,

I now have a preliminary fix this.

Turning the crypto and neon features on only in the assembler turned out to be an easy thing to do. However, correctly handling the inline assembler constraints is more difficult and error-prone (because the simd registers now hold illegal types).

Do you have other examples where this feature is used? I would like to do some more testing before putting this out for review.

Thanks,
Silviu
Comment 4 Pirama Arumuga Nainar 2016-11-16 18:32:07 PST
Hi Silviu, did you try the inline assembly in https://android.googlesource.com/kernel/tegra/+/android-tegra-3.10/arch/arm64/crypto/aes-ce-cipher.c?  There are two assembly blocks there that clobber v0 and v1.

Unfortunately, I couldn't find any other example use case.
Comment 5 Silviu Baranga 2016-11-16 18:43:52 PST
Thanks, Pirama!

Yes, I went through all of the use cases there. One of them has an umov that clang doesn't like, but otherwise they all work.


In that case I'll put the patches up for review.

-Silviu
Comment 6 Tim Northover 2016-11-18 13:51:00 PST
The only difference between this and -mno-implicit-float (which works without adding extra backend features) seems to be how it handles explicit floating-point code.

-mno-implicit-float allows it; -mgeneral-regs-only errors on GCC (it's even polite enough to apologize!), but tries to form some new horribly unsupported soft-float ABI on Clang.

I'd suggest switching it to set NoImplicitFloat instead to fix this particular issue, and then perhaps working out how it should behave in the front-end (error, == -mfloat-abi=soft, or floats allowed).
Comment 7 Pirama Arumuga Nainar 2016-11-29 01:19:58 PST
Tim, thanks for the pointer to use -mno-implicit-float.  It works for the Kernel because this particular file does not have any explicit floating point operations.

I also agree with the discussion in https://reviews.llvm.org/D26856 that -mgeneral-regs-only should reject explicit floating point code.
Comment 8 Manoj Gupta 2017-04-24 10:44:25 PDT
Hi Silviu,

Are you planning on fixing this? I see that https://reviews.llvm.org/D26856 has not been updated in a while.
Comment 9 Silviu Baranga 2017-04-25 07:46:57 PDT
Hi Manoj,

The solution at D26856 was the wrong one, we need to reject FP uses in front-end (so probably D26856 could be abandoned).

I don't have any plans to implement this at the moment.

However, we should have a work-around with -mno-implicit-float (as long as the code doesn't use any floating point types).

Thanks,
Silviu
Comment 10 Nick Desaulniers 2017-09-15 11:10:08 PDT
For anyone interested in picking this up, this is _the last_ bug preventing us from compiling the upstream Linux kernel with LLVM for aarch64 without any out of tree patches.

For more information, please see:
* https://www.linuxplumbersconf.org/2017/ocw//system/presentations/4799/original/LPC%202017-%20Clang%20built%20kernels.pdf
* https://chromium.googlesource.com/chromiumos/third_party/kernel/+/1760910682ce3750f775452b8f97872b4e3ed808%5E%21/#F0
* https://lkml.org/lkml/2017/8/22/912
Comment 11 Manoj Gupta 2017-09-15 11:11:57 PDT
George, Are you working on it?
Comment 12 George Burgess 2017-09-15 11:19:07 PDT
Yeah, I have a half-made patch that tries to address this. Was planning on sending it out by the end of next week.
Comment 13 George Burgess 2017-10-02 14:49:14 PDT
Proposed fix: https://reviews.llvm.org/D38479