[AArch64] Make .arch without extra features actually take effect

[mstorsjo]

Summary

This fixes PR32873 / https://github.com/llvm/llvm-project/issues/32220.

@ab mentioned in that bug report that he would send a patch for the issue
soon, but that apparently didn't land at least, and I don't find any
from quickly browsing the mailing list archives after that time.

[pratlucas]

LGTM.

[DavidSpickett]

• Is there any use case for the previous behaviour that would be broken by this?
• Is it worth adding a release note for? (regardless of the answer to the above)
• Does gcc/gas actually behave in the way this patch implements?
• Do you know if other backends have this problem? (does not need to be included in this patch however)

Besides that, no objections to fixing this.

[mstorsjo]

In D151982#4390604, @DavidSpickett wrote:

• Is there any use case for the previous behaviour that would be broken by this?

Not a direct use case per se, but this does affect how things do work out in some cases.

https://patchwork.ffmpeg.org/project/ffmpeg/patch/[e-mail]/ is a patch for ffmpeg, that tries to allow assembling the dotprod/i8mm extensions (in handwritten assembly) even if the baseline target doesn't include those extensions. Support for those extensions will be detected at runtime, and functions that rely on those extensions will be used if supported, while the binaries in general can run on anything from ARMv8.0 upwards.

To achieve this, the patch detects whether .arch armv8.2-a is supported, and whether .arch_extension i8mm is supported. The .arch directive is required, since binutils refuses to enable those extensions unless the base architecture level is >= 8.2.

So for most cases, we're expected to end up with this:

.arch armv8.2-a
.arch_extension dotprod
.arch_extension i8mm
<dotprod/i8mm instructions>

However dotprod and i8mm aren't supported in .arch_extension right now (fixed by D151981), but .arch armv8.2-a is. Now if we're building with -march=armv8.6-a, we do support those extensions without any directives.

So with current released Clang, we get this, if building without -march=armv8.6-a:

.arch armv8.2-a
// No dotprod/i8mm instructions used, LLVM is unable to enable them via assembly source

If building with -march=armv8.6-a, we get this:

.arch armv8.2-a
// Dotprod/i8mm instructions used since the .arch doesn't have any effect

With this patch applied, we wouldn't be able to use those instructions here. But coupled with D151981 we can, and we can also make use of them without -march=armv8.6-a.

But this patch also makes it possible to enable the use of those instructions with .arch armv8.6-a in the assembly source, which didn't work before.

So TL;DR, this patch _does_ change how things behave. But it does change things towards a more correct and expected behaviour - I don't see the previous behaviour as an expected behaviour that anyone would rely on, especially if they expect to assemble with binutils too.

• Is it worth adding a release note for? (regardless of the answer to the above)

I guess so

• Does gcc/gas actually behave in the way this patch implements?

Yes; this patch makes LLVM behave like binutils gas in all of those cases as mentioned above.

• Do you know if other backends have this problem? (does not need to be included in this patch however)

Not sure offhand, dunno if .arch (or other similar directives) behaves in a similar way on other architectures.

[DavidSpickett]

especially if they expect to assemble with binutils too.

Didn't think about it that way, this makes sense.

Not sure offhand, dunno if .arch (or other similar directives) behaves in a similar way on other architectures.

I forgot this is somewhat Arm specific. I see https://sourceware.org/binutils/docs/as/ARM-Directives.html, I'll check what we do for that.

[DavidSpickett]

Arm works as expected:

void fn() {
    __asm__ volatile (".arch_extension sec\n"
                      ".arch thumbv7\n"
                      "smc #0\n");
}

<source>:4:24: error: instruction requires: TrustZone
                      "smc #0\n");
                       ^

.arch then .arch_extension compiles without error.

Intel does recognise the directive but it (deliberately) ignores it.

[mstorsjo]

Added a release note.

[DavidSpickett]

LGTM

llvm/docs/ReleaseNotes.rst

 Changes to the AArch64 Backend
 ------------------------------
 
 * Added Assembly Support for the 2022 A-profile extensions FEAT_GCS (Guarded
   Control Stacks), FEAT_CHK (Check Feature Status), and FEAT_ATS1A.
 * Support for preserve_all calling convention is added.
 * Added support for missing arch extensions in the assembly directives
   ``.arch <level>+<ext>`` and ``.arch_extension``.
+* Fixed handling of ``.arch <level>`` in assembly, without using any ``+<ext>``
+  suffix. Previously this had no effect at all if no extensions were supplied.
+  Now ``.arch <level>`` can be used to enable all the extensions that are
+  included in a higher level than what is specified on the command line,
+  or for disabling unwanted extensions if setting it to a lower level.
+  This fixes `PR32873 <https://github.com/llvm/llvm-project/issues/32220>`.
 
 Changes to the AMDGPU Backend
 -----------------------------
 * More fine-grained synchronization around barriers for newer architectures
   (gfx90a+, gfx10+). The AMDGPU backend now omits previously automatically
   generated waitcnt instructions before barriers, allowing for more precise
   control. Users must now use memory fences to implement fine-grained
   synchronization strategies around barriers. Refer to `AMDGPU memory model

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

bool AArch64AsmParser::parseDirectiveArch(SMLoc L) {
 
   SmallVector<StringRef, 4> RequestedExtensions;
   if (!ExtensionString.empty())
     ExtensionString.split(RequestedExtensions, '+');
 
   ExpandCryptoAEK(*ArchInfo, RequestedExtensions);
 
   FeatureBitset Features = STI.getFeatureBits();
+  setAvailableFeatures(ComputeAvailableFeatures(Features));
   for (auto Name : RequestedExtensions) {
     bool EnableFeature = true;
 
     if (Name.starts_with_insensitive("no")) {
       EnableFeature = false;
       Name = Name.substr(2);
     }
 

llvm/test/MC/AArch64/directive-arch-negative.s

 # CHECK-NEXT: ^
 
   .arch armv8.1-a+noras
   esb
 
 # CHECK: error: instruction requires: ras
 # CHECK-NEXT:   esb
 
-// PR32873: without extra features, '.arch' is currently ignored.
-// Add an unrelated feature to accept the directive.
-  .arch armv8+crc
+  .arch armv8
         casa  w5, w7, [x19]
 
 # CHECK: error: instruction requires: lse
 # CHECK-NEXT:   casa  w5, w7, [x19]
 
   .arch armv8+crypto
         crc32b w0, w1, w2
 
 # CHECK: error: instruction requires: crc
 # CHECK-NEXT:   crc32b w0, w1, w2
 
   .arch armv8.1-a+nolse
         casa  w5, w7, [x20]
 
 # CHECK: error: instruction requires: lse
 # CHECK-NEXT:   casa  w5, w7, [x20]

llvm/test/MC/AArch64/directive-arch.s

 // RUN: llvm-mc -triple aarch64-unknown-none-eabi -filetype asm -o - %s 2>&1 | FileCheck %s
 
   .arch armv8-a+crypto
 
   aesd v0.16b, v2.16b
   eor v0.16b, v0.16b, v2.16b
 
 # CHECK:  aesd  v0.16b, v2.16b
 # CHECK:        eor     v0.16b, v0.16b, v2.16b
 
-// PR32873: without extra features, '.arch' is currently ignored.
-// Add an unrelated feature to accept the directive.
-  .arch armv8.1-a+crypto
+  .arch armv8.1-a
         casa  w5, w7, [x20]
 # CHECK:        casa    w5, w7, [x20]
 
   .arch armv8-a+lse
   casa  w5, w7, [x20]
 # CHECK:        casa    w5, w7, [x20]
 
   .arch armv8.5-a+rng
   mrs   x0, rndr
   mrs   x0, rndrrs
 # CHECK:        mrs     x0, RNDR
 # CHECK:        mrs     x0, RNDRRS