[AArch64] Support optional constant offset for constraint "S" #80255

MaskRay · 2024-02-01T07:25:26Z

Modify the initial implementation (https://reviews.llvm.org/D46745) to
support a constant offset so that the following code will compile:

int a[2][2];
void foo() { asm("// %0" :: "S"(&a[1][1])); }

We use the generic code path for "s". In GCC's aarch64 port, "S" is
supported for PIC while "s" isn't, making "s" less useful. We implement
"S" but not "s".

Similar to #80201 for RISC-V.

Created using spr 1.3.4

llvmbot · 2024-02-01T07:25:56Z

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-aarch64

Author: Fangrui Song (MaskRay)

Changes

Modify the initial implementation (https://reviews.llvm.org/D46745) to
support a constant offset so that the following code will compile:

int a[2][2];
void foo() { asm("// %0" :: "S"(&amp;a[1][1])); }

We use the generic code path for "s". In GCC's aarch64 port, "S" is
supported for PIC while "s" isn't, making "s" less useful. We implement
"S" but not "s".

Similar to #80201 for RISC-V.

Full diff: https://github.com/llvm/llvm-project/pull/80255.diff

5 Files Affected:

(modified) clang/test/Sema/inline-asm-validate-aarch64.c (+14-3)
(modified) llvm/docs/LangRef.rst (+2)
(modified) llvm/lib/Target/AArch64/AArch64ISelLowering.cpp (+6-13)
(modified) llvm/test/CodeGen/AArch64/inlineasm-S-constraint.ll (+4)
(modified) llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll (+1)

diff --git a/clang/test/Sema/inline-asm-validate-aarch64.c b/clang/test/Sema/inline-asm-validate-aarch64.c
index 014767d5a3923..1e753d40d8ca0 100644
--- a/clang/test/Sema/inline-asm-validate-aarch64.c
+++ b/clang/test/Sema/inline-asm-validate-aarch64.c
@@ -1,14 +1,24 @@
+// RUN: %clang_cc1 -triple aarch64 -fsyntax-only -verify -DVERIFY %s
 // RUN: %clang_cc1 -triple arm64-apple-darwin -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 typedef unsigned char uint8_t;
 
+#ifdef VERIFY
+void test_s(int i) {
+  asm("" :: "s"(i)); // expected-error{{invalid input constraint 's' in asm}}
+
+  /// Codegen error
+  asm("" :: "S"(i));
+  asm("" :: "S"(test_s(i))); // expected-error{{invalid type 'void' in asm input for constraint 'S'}}
+}
+#else
 uint8_t constraint_r(uint8_t *addr) {
   uint8_t byte;
 
   __asm__ volatile("ldrb %0, [%1]" : "=r" (byte) : "r" (addr) : "memory");
 // CHECK: warning: value size does not match register size specified by the constraint and modifier
 // CHECK: note: use constraint modifier "w"
-// CHECK: fix-it:{{.*}}:{8:26-8:28}:"%w0"
+// CHECK: fix-it:{{.*}}:{[[#@LINE-3]]:26-[[#@LINE-3]]:28}:"%w0"
 
   return byte;
 }
@@ -19,7 +29,7 @@ uint8_t constraint_r_symbolic(uint8_t *addr) {
   __asm__ volatile("ldrb %[s0], [%[s1]]" : [s0] "=r" (byte) : [s1] "r" (addr) : "memory");
 // CHECK: warning: value size does not match register size specified by the constraint and modifier
 // CHECK: note: use constraint modifier "w"
-// CHECK: fix-it:{{.*}}:{19:26-19:31}:"%w[s0]"
+// CHECK: fix-it:{{.*}}:{[[#@LINE-3]]:26-[[#@LINE-3]]:31}:"%w[s0]"
 
   return byte;
 }
@@ -40,11 +50,11 @@ uint8_t constraint_r_symbolic_macro(uint8_t *addr) {
 // CHECK: warning: value size does not match register size specified by the constraint and modifier
 // CHECK: asm ("%w0 %w1 %2" : "+r" (one) : "r" (wide_two));
 // CHECK: note: use constraint modifier "w"
-// CHECK: fix-it:{{.*}}:{47:17-47:19}:"%w2"
 
 void read_write_modifier0(int one, int two) {
   long wide_two = two;
   asm ("%w0 %w1 %2" : "+r" (one) : "r" (wide_two));
+// CHECK: fix-it:{{.*}}:{[[#@LINE-1]]:17-[[#@LINE-1]]:19}:"%w2"
 }
 
 // CHECK-NOT: warning: 
@@ -52,3 +62,4 @@ void read_write_modifier1(int one, int two) {
   long wide_two = two;
   asm ("%w0 %1" : "+r" (one), "+r" (wide_two));
 }
+#endif
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 7a7ddc59ba985..b13f8c7811d16 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -5107,6 +5107,8 @@ AArch64:
   offsets). (However, LLVM currently does this for the ``m`` constraint as
   well.)
 - ``r``: A 32 or 64-bit integer register (W* or X*).
+- ``S``: A symbol or label reference with a constant offset. The generic ``s``
+  is not supported.
 - ``Uci``: Like r, but restricted to registers 8 to 11 inclusive.
 - ``Ucj``: Like r, but restricted to registers 12 to 15 inclusive.
 - ``w``: A 32, 64, or 128-bit floating-point, SIMD or SVE vector register.
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index bb19aef978b94..62f160c1c33fc 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -10618,7 +10618,7 @@ AArch64TargetLowering::getConstraintType(StringRef Constraint) const {
     case 'Z':
       return C_Immediate;
     case 'z':
-    case 'S': // A symbolic address
+    case 'S': // A symbol or label reference with a constant offset
       return C_Other;
     }
   } else if (parsePredicateConstraint(Constraint))
@@ -10801,19 +10801,12 @@ void AArch64TargetLowering::LowerAsmOperandForConstraint(
       Result = DAG.getRegister(AArch64::WZR, MVT::i32);
     break;
   }
-  case 'S': {
-    // An absolute symbolic address or label reference.
-    if (const GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(Op)) {
-      Result = DAG.getTargetGlobalAddress(GA->getGlobal(), SDLoc(Op),
-                                          GA->getValueType(0));
-    } else if (const BlockAddressSDNode *BA =
-                   dyn_cast<BlockAddressSDNode>(Op)) {
-      Result =
-          DAG.getTargetBlockAddress(BA->getBlockAddress(), BA->getValueType(0));
-    } else
-      return;
+  case 'S':
+    // Use the generic code path for "s". In GCC's aarch64 port, "S" is
+    // supported for PIC while "s" isn't, making "s" less useful. We implement
+    // "S" but not "s".
+    TargetLowering::LowerAsmOperandForConstraint(Op, "s", Ops, DAG);
     break;
-  }
 
   case 'I':
   case 'J':
diff --git a/llvm/test/CodeGen/AArch64/inlineasm-S-constraint.ll b/llvm/test/CodeGen/AArch64/inlineasm-S-constraint.ll
index 18b4ef955236a..7664ad16dd568 100644
--- a/llvm/test/CodeGen/AArch64/inlineasm-S-constraint.ll
+++ b/llvm/test/CodeGen/AArch64/inlineasm-S-constraint.ll
@@ -1,11 +1,15 @@
 ;RUN: llc -mtriple=aarch64-none-linux-gnu -mattr=+neon < %s | FileCheck %s
 @var = global i32 0
+@a = external global [2 x [2 x i32]], align 4
+
 define void @test_inline_constraint_S() {
 ; CHECK-LABEL: test_inline_constraint_S:
   call void asm sideeffect "adrp x0, $0", "S"(ptr @var)
   call void asm sideeffect "add x0, x0, :lo12:$0", "S"(ptr @var)
+  call void asm sideeffect "// $0", "S"(ptr getelementptr inbounds ([2 x [2 x i32]], ptr @a, i64 0, i64 1, i64 1))
 ; CHECK: adrp x0, var
 ; CHECK: add x0, x0, :lo12:var
+; CHECK: // a+12
   ret void
 }
 define i32 @test_inline_constraint_S_label(i1 %in) {
diff --git a/llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll b/llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll
index 11add36da27b6..a54cc3c14e741 100644
--- a/llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll
+++ b/llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll
@@ -3,6 +3,7 @@
 ; RUN: llc -mtriple=riscv64 < %s | FileCheck %s --check-prefix=RV64
 
 @var = external dso_local global i32, align 4
+@a = external global [2 x [2 x i32]], align 4
 
 define dso_local ptr @constraint_S() {
 ; RV32-LABEL: constraint_S:

MaskRay · 2024-02-01T07:27:19Z

I have some notes about these constraints https://maskray.me/blog/2024-01-30-raw-symbol-names-in-inline-assembly

smithp35 · 2024-02-01T13:53:06Z

Just to make sure I've understood:

x86_64's support for constraint "s" was hoisted into the target independent TargetLowering in https://reviews.llvm.org/D61560
Clang's implementation of constraint "s" isn't restricted to non-PIC/non-PIE like GCC's
We can implement constraint "S" in terms of constraint "s" as Clang's implementation of "s" is equivalent to AArch64 "S".

On that basis this looks reasonable to me. I've not had time to pick over the details of the code/test changes though.

MaskRay · 2024-02-01T17:36:05Z

Just to make sure I've understood:

x86_64's support for constraint "s" was hoisted into the target independent TargetLowering in reviews.llvm.org/D61560

Clang's implementation of constraint "s" isn't restricted to non-PIC/non-PIE like GCC's

We can implement constraint "S" in terms of constraint "s" as Clang's implementation of "s" is equivalent to AArch64 "S".

Yes, these are all correct.

On that basis this looks reasonable to me. I've not had time to pick over the details of the code/test changes though.

Thanks!

smithp35

Thanks for the response. LGTM.

I think the RISCV inline-asm-S-constraint.ll is no longer necessary thanks to #80201

smithp35 · 2024-02-02T10:49:32Z

llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll

@@ -3,6 +3,7 @@
 ; RUN: llc -mtriple=riscv64 < %s | FileCheck %s --check-prefix=RV64

 @var = external dso_local global i32, align 4
+@a = external global [2 x [2 x i32]], align 4


Did you mean to change this file in this PR? I think this change has already been made in #80201

Sorry, no. I accidentally included this when preparing tests using my experience on x86 Ws and RISC-V S:)

Created using spr 1.3.4

…0255) Modify the initial implementation (https://reviews.llvm.org/D46745) to support a constant offset so that the following code will compile: ``` int a[2][2]; void foo() { asm("// %0" :: "S"(&a[1][1])); } ``` We use the generic code path for "s". In GCC's aarch64 port, "S" is supported for PIC while "s" isn't, making "s" less useful. We implement "S" but not "s". Similar to llvm#80201 for RISC-V.

The generic constraint "i" seems to be copied from x86 or arm (and with a redundant generic operand modifier "c"). It works with -fno-PIE but not with -fPIE/-fPIC in GCC's aarch64 port. The machine constraint "S", which denotes a symbol or label reference with a constant offset, supports PIC and has been available in GCC since 2012 and in Clang since 7.0. However, Clang before 19 does not support "S" on a symbol with a constant offset [1] (e.g. `static_key_false(&nf_hooks_needed[pf][hook])` in include/linux/netfilter.h), so we use "i" as a fallback. Suggested-by: Ard Biesheuvel <[email protected]> Signed-off-by: Fangrui Song <[email protected]> Link: llvm/llvm-project#80255 [1] Acked-by: Mark Rutland <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Will Deacon <[email protected]>

[𝘀𝗽𝗿] initial version

8ce25b5

Created using spr 1.3.4

llvmbot added clang Clang issues not falling into any other category backend:AArch64 llvm:ir labels Feb 1, 2024

MaskRay requested a review from smithp35 February 1, 2024 07:25

MaskRay requested review from davemgreen and TNorthover February 1, 2024 07:26

smithp35 approved these changes Feb 2, 2024

View reviewed changes

remove RISC-V S test

3fac3a9

Created using spr 1.3.4

MaskRay merged commit d4de4c3 into main Feb 2, 2024

MaskRay deleted the users/MaskRay/spr/aarch64-support-optional-constant-offset-for-constraint-s branch February 2, 2024 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AArch64] Support optional constant offset for constraint "S" #80255

[AArch64] Support optional constant offset for constraint "S" #80255

Uh oh!

MaskRay commented Feb 1, 2024

Uh oh!

llvmbot commented Feb 1, 2024 •

edited

Loading

Uh oh!

MaskRay commented Feb 1, 2024

Uh oh!

smithp35 commented Feb 1, 2024

Uh oh!

MaskRay commented Feb 1, 2024

Uh oh!

smithp35 left a comment

Uh oh!

smithp35 Feb 2, 2024

Uh oh!

MaskRay Feb 2, 2024

Uh oh!

Uh oh!

[AArch64] Support optional constant offset for constraint "S" #80255

[AArch64] Support optional constant offset for constraint "S" #80255

Uh oh!

Conversation

MaskRay commented Feb 1, 2024

Uh oh!

llvmbot commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaskRay commented Feb 1, 2024

Uh oh!

smithp35 commented Feb 1, 2024

Uh oh!

MaskRay commented Feb 1, 2024

Uh oh!

smithp35 left a comment

Choose a reason for hiding this comment

Uh oh!

smithp35 Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Feb 1, 2024 •

edited

Loading