[RISCV] Support constraint "s" #80201

MaskRay · 2024-01-31T21:39:14Z

GCC has supported a generic constraint "s" for a long time (since at
least 1992), which references a symbol or label with an optional
constant offset. "i" is a superset that also supports a constant
integer.

GCC's RISC-V port also supports a machine-specific constraint "S",
which cannot be used with a preemptible symbol. (We don't bother to
check preemptibility.) In PIC code, an external symbol is preemptible by
default, making "S" less useful if you want to create an artificial
reference for linker garbage collection, or define sections to hold
symbol addresses:

void fun();
// error: impossible constraint in ‘asm’ for riscv64-linux-gnu-gcc -fpie/-fpic
void foo() { asm(".reloc ., BFD_RELOC_NONE, %0" :: "S"(fun)); }
// good even if -fpie/-fpic
void foo() { asm(".reloc ., BFD_RELOC_NONE, %0" :: "s"(fun)); }

This patch adds support for "s". Modify https://reviews.llvm.org/D105254
("S") to handle multi-depth GEPs (https://reviews.llvm.org/D61560).

Created using spr 1.3.4

llvmbot · 2024-01-31T21:39:45Z

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

@llvm/pr-subscribers-backend-risc-v

Author: Fangrui Song (MaskRay)

Changes

GCC has supported a generic constraint "s" for a long time (since at
least 1992), which references a symbol or label with an optional
constant offset. "i" is a superset that also supports a constant
integer.

GCC's RISC-V port also supports a machine-specific constraint "S",
which cannot be used with a preemptible symbol. (We don't bother to
check preemptibility.) In PIC code, an external symbol is preemptible by
default, making "S" less useful if you want to create an artificial
reference for linker garbage collection, or define sections to hold
symbol addresses:

void fun();
// error: impossible constraint in ‘asm’ for riscv64-linux-gnu-gcc -fpie/-fpic
void foo() { asm(".reloc ., BFD_RELOC_NONE, %0" :: "S"(fun)); }
// good even if -fpie/-fpic
void foo() { asm(".reloc ., BFD_RELOC_NONE, %0" :: "s"(fun)); }

This patch adds support for "s". Modify https://reviews.llvm.org/D105254
("S") to handle multi-depth GEPs (https://reviews.llvm.org/D61560).

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

8 Files Affected:

(modified) clang/lib/Basic/Targets/RISCV.cpp (+2-1)
(modified) clang/test/CodeGen/RISCV/riscv-inline-asm.c (+12-4)
(modified) clang/test/Sema/inline-asm-validate-riscv.c (+8)
(modified) llvm/docs/LangRef.rst (+2-1)
(modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+2-7)
(removed) llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll (-54)
(added) llvm/test/CodeGen/RISCV/inline-asm-s-constraint-error.ll (+14)
(added) llvm/test/CodeGen/RISCV/inline-asm-s-constraint.ll (+76)

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index c71b2e9eeb6c1..837a6e799e3a9 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -96,7 +96,8 @@ bool RISCVTargetInfo::validateAsmConstraint(
     // An address that is held in a general-purpose register.
     Info.setAllowsMemory();
     return true;
-  case 'S': // A symbolic address
+  case 's':
+  case 'S': // A symbol or label reference with a constant offset
     Info.setAllowsRegister();
     return true;
   case 'v':
diff --git a/clang/test/CodeGen/RISCV/riscv-inline-asm.c b/clang/test/CodeGen/RISCV/riscv-inline-asm.c
index 48de5ba3a27e3..3565705dea713 100644
--- a/clang/test/CodeGen/RISCV/riscv-inline-asm.c
+++ b/clang/test/CodeGen/RISCV/riscv-inline-asm.c
@@ -45,8 +45,16 @@ void test_A(int *p) {
   asm volatile("" :: "A"(*p));
 }
 
-void test_S(void) {
-// CHECK-LABEL: define{{.*}} void @test_S()
-// CHECK: call void asm sideeffect "", "S"(ptr nonnull @f)
-  asm volatile("" :: "S"(&f));
+extern int var, arr[2][2];
+struct Pair { int a, b; } pair;
+
+// CHECK-LABEL: test_s(
+// CHECK:         call void asm sideeffect "// $0 $1 $2", "s,s,s"(ptr nonnull @var, ptr nonnull getelementptr inbounds ([2 x [2 x i32]], ptr @arr, {{.*}}), ptr nonnull @test_s)
+// CHECK:         call void asm sideeffect "// $0", "s"(ptr nonnull getelementptr inbounds (%struct.Pair, ptr @pair, {{.*}}))
+// CHECK:         call void asm sideeffect "// $0 $1 $2", "S,S,S"(ptr nonnull @var, ptr nonnull getelementptr inbounds ([2 x [2 x i32]], ptr @arr, {{.*}}), ptr nonnull @test_s)
+void test_s(void) {
+  asm("// %0 %1 %2" :: "s"(&var), "s"(&arr[1][1]), "s"(test_s));
+  asm("// %0" :: "s"(&pair.b));
+
+  asm("// %0 %1 %2" :: "S"(&var), "S"(&arr[1][1]), "S"(test_s));
 }
diff --git a/clang/test/Sema/inline-asm-validate-riscv.c b/clang/test/Sema/inline-asm-validate-riscv.c
index 43a5378bc3f25..806ef60dacbec 100644
--- a/clang/test/Sema/inline-asm-validate-riscv.c
+++ b/clang/test/Sema/inline-asm-validate-riscv.c
@@ -22,6 +22,14 @@ void K(int k) {
   asm volatile ("" :: "K"(AboveMax)); // expected-error{{value '32' out of range for constraint 'K'}}
 }
 
+void test_s(int i) {
+  asm("" :: "s"(test_s(0))); // expected-error{{invalid type 'void' in asm input for constraint 's'}}
+  /// Codegen error
+  asm("" :: "s"(i));
+
+  asm("" :: "S"(test_s(0))); // expected-error{{invalid type 'void' in asm input for constraint 'S'}}
+}
+
 void test_clobber_conflict(void) {
   register long x10 asm("x10");
   asm volatile("" :: "r"(x10) : "x10"); // expected-error {{conflicts with asm clobber list}}
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 7a7ddc59ba985..3648ea2611dd4 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -5075,7 +5075,7 @@ Some constraint codes are typically supported by all targets:
 - ``i``: An integer constant (of target-specific width). Allows either a simple
   immediate, or a relocatable value.
 - ``n``: An integer constant -- *not* including relocatable values.
-- ``s``: An integer constant, but allowing *only* relocatable values.
+- ``s``: A symbol or label reference with a constant offset.
 - ``X``: Allows an operand of any kind, no constraint whatsoever. Typically
   useful to pass a label for an asm branch or call.
 
@@ -5283,6 +5283,7 @@ RISC-V:
 - ``f``: A 32- or 64-bit floating-point register (requires F or D extension).
 - ``r``: A 32- or 64-bit general-purpose register (depending on the platform
   ``XLEN``).
+- ``S``: Alias for ``s``.
 - ``vr``: A vector register. (requires V extension).
 - ``vm``: A vector register for masking operand. (requires V extension).
 
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index b8994e7b7bdb2..fcddf7234e5fa 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -19195,6 +19195,7 @@ RISCVTargetLowering::getConstraintType(StringRef Constraint) const {
       return C_Immediate;
     case 'A':
       return C_Memory;
+    case 's':
     case 'S': // A symbolic address
       return C_Other;
     }
@@ -19456,13 +19457,7 @@ void RISCVTargetLowering::LowerAsmOperandForConstraint(
       }
       return;
     case 'S':
-      if (const auto *GA = dyn_cast<GlobalAddressSDNode>(Op)) {
-        Ops.push_back(DAG.getTargetGlobalAddress(GA->getGlobal(), SDLoc(Op),
-                                                 GA->getValueType(0)));
-      } else if (const auto *BA = dyn_cast<BlockAddressSDNode>(Op)) {
-        Ops.push_back(DAG.getTargetBlockAddress(BA->getBlockAddress(),
-                                                BA->getValueType(0)));
-      }
+      TargetLowering::LowerAsmOperandForConstraint(Op, "s", Ops, DAG);
       return;
     default:
       break;
diff --git a/llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll b/llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll
deleted file mode 100644
index 11add36da27b6..0000000000000
--- a/llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll
+++ /dev/null
@@ -1,54 +0,0 @@
-; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=riscv32 < %s | FileCheck %s --check-prefix=RV32
-; RUN: llc -mtriple=riscv64 < %s | FileCheck %s --check-prefix=RV64
-
-@var = external dso_local global i32, align 4
-
-define dso_local ptr @constraint_S() {
-; RV32-LABEL: constraint_S:
-; RV32:       # %bb.0:
-; RV32-NEXT:    #APP
-; RV32-NEXT:    lui a0, %hi(var)
-; RV32-NEXT:    addi a0, a0, %lo(var)
-; RV32-NEXT:    #NO_APP
-; RV32-NEXT:    ret
-;
-; RV64-LABEL: constraint_S:
-; RV64:       # %bb.0:
-; RV64-NEXT:    #APP
-; RV64-NEXT:    lui a0, %hi(var)
-; RV64-NEXT:    addi a0, a0, %lo(var)
-; RV64-NEXT:    #NO_APP
-; RV64-NEXT:    ret
-  %ret = tail call ptr asm "lui $0, %hi($1)\0Aaddi $0, $0, %lo($1)", "=r,S"(ptr nonnull @var)
-  ret ptr %ret
-}
-
-; Function Attrs: nofree nosync nounwind readnone
-define dso_local ptr @constraint_S_label() {
-; RV32-LABEL: constraint_S_label:
-; RV32:       # %bb.0: # %entry
-; RV32-NEXT:  .Ltmp0: # Block address taken
-; RV32-NEXT:  # %bb.1: # %L1
-; RV32-NEXT:    #APP
-; RV32-NEXT:    lui a0, %hi(.Ltmp0)
-; RV32-NEXT:    addi a0, a0, %lo(.Ltmp0)
-; RV32-NEXT:    #NO_APP
-; RV32-NEXT:    ret
-;
-; RV64-LABEL: constraint_S_label:
-; RV64:       # %bb.0: # %entry
-; RV64-NEXT:  .Ltmp0: # Block address taken
-; RV64-NEXT:  # %bb.1: # %L1
-; RV64-NEXT:    #APP
-; RV64-NEXT:    lui a0, %hi(.Ltmp0)
-; RV64-NEXT:    addi a0, a0, %lo(.Ltmp0)
-; RV64-NEXT:    #NO_APP
-; RV64-NEXT:    ret
-entry:
-  br label %L1
-
-L1:
-  %ret = tail call ptr asm "lui $0, %hi($1)\0Aaddi $0, $0, %lo($1)", "=r,S"(ptr blockaddress(@constraint_S_label, %L1))
-  ret ptr %ret
-}
diff --git a/llvm/test/CodeGen/RISCV/inline-asm-s-constraint-error.ll b/llvm/test/CodeGen/RISCV/inline-asm-s-constraint-error.ll
new file mode 100644
index 0000000000000..f836dd6366229
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/inline-asm-s-constraint-error.ll
@@ -0,0 +1,14 @@
+; RUN: not llc -mtriple=riscv64 < %s 2>&1 | FileCheck %s
+
+@a = external global [4 x i32], align 16
+
+; CHECK-COUNT-2: error: invalid operand for inline asm constraint 's'
+; CHECK-NOT:     error:
+define void @test(i64 %i) {
+entry:
+  %x = alloca i32, align 4
+  %ai = getelementptr inbounds [4 x i32], ptr @a, i64 0, i64 %i
+  call void asm sideeffect "", "s,~{dirflag},~{fpsr},~{flags}"(ptr %x)
+  call void asm sideeffect "", "s,~{dirflag},~{fpsr},~{flags}"(ptr %ai)
+  ret void
+}
diff --git a/llvm/test/CodeGen/RISCV/inline-asm-s-constraint.ll b/llvm/test/CodeGen/RISCV/inline-asm-s-constraint.ll
new file mode 100644
index 0000000000000..4a6c47f88014c
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/inline-asm-s-constraint.ll
@@ -0,0 +1,76 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -relocation-model=static < %s | FileCheck %s --check-prefix=RV32
+; RUN: llc -mtriple=riscv64 -relocation-model=pic < %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 void @test() {
+; CHECK-LABEL: test:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    # var a+12 test
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    ret{{[l|q]}}
+; RV32-LABEL: test:
+; RV32:       # %bb.0: # %entry
+; RV32-NEXT:    #APP
+; RV32-NEXT:    # var a+12 test
+; RV32-NEXT:    #NO_APP
+; RV32-NEXT:    #APP
+; RV32-NEXT:    # var a+12 test
+; RV32-NEXT:    #NO_APP
+; RV32-NEXT:    ret
+;
+; RV64-LABEL: test:
+; RV64:       # %bb.0: # %entry
+; RV64-NEXT:    #APP
+; RV64-NEXT:    # var a+12 .Ltest$local
+; RV64-NEXT:    #NO_APP
+; RV64-NEXT:    #APP
+; RV64-NEXT:    # var a+12 .Ltest$local
+; RV64-NEXT:    #NO_APP
+; RV64-NEXT:    ret
+entry:
+  call void asm sideeffect "// $0 $1 $2", "s,s,s,~{dirflag},~{fpsr},~{flags}"(ptr @var, ptr getelementptr inbounds ([2 x [2 x i32]], ptr @a, i64 0, i64 1, i64 1), ptr @test)
+
+  ;; Implement "S" as an alias for "s".
+  call void asm sideeffect "// $0 $1 $2", "S,S,S,~{dirflag},~{fpsr},~{flags}"(ptr @var, ptr getelementptr inbounds ([2 x [2 x i32]], ptr @a, i64 0, i64 1, i64 1), ptr @test)
+  ret void
+}
+
+; Function Attrs: nofree nosync nounwind readnone
+define dso_local ptr @test_label() {
+; RV32-LABEL: test_label:
+; RV32:       # %bb.0: # %entry
+; RV32-NEXT:  .Ltmp0: # Block address taken
+; RV32-NEXT:  # %bb.1: # %L1
+; RV32-NEXT:    #APP
+; RV32-NEXT:    # .Ltmp0
+; RV32-NEXT:    #NO_APP
+; RV32-NEXT:    #APP
+; RV32-NEXT:    lui a0, %hi(.Ltmp0)
+; RV32-NEXT:    addi a0, a0, %lo(.Ltmp0)
+; RV32-NEXT:    #NO_APP
+; RV32-NEXT:    ret
+;
+; RV64-LABEL: test_label:
+; RV64:       # %bb.0: # %entry
+; RV64-NEXT:  .Ltmp0: # Block address taken
+; RV64-NEXT:  # %bb.1: # %L1
+; RV64-NEXT:    #APP
+; RV64-NEXT:    # .Ltmp0
+; RV64-NEXT:    #NO_APP
+; RV64-NEXT:    #APP
+; RV64-NEXT:    lui a0, %hi(.Ltmp0)
+; RV64-NEXT:    addi a0, a0, %lo(.Ltmp0)
+; RV64-NEXT:    #NO_APP
+; RV64-NEXT:    ret
+entry:
+  br label %L1
+
+L1:
+  call void asm sideeffect "// $0", "s,~{dirflag},~{fpsr},~{flags}"(ptr blockaddress(@test_label, %L1))
+  %ret = tail call ptr asm "lui $0, %hi($1)\0Aaddi $0, $0, %lo($1)", "=r,S"(ptr blockaddress(@test_label, %L1))
+  ret ptr %ret
+}

MaskRay · 2024-01-31T22:11:24Z

The discussion with GCC seems to suggest we should encourage "s" for metadata section usage.

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

While the default implementation (gcc/defaults.h) is permissive (used by MIPS, PowerPC, and RISC-V), many ports impose stricter restrictions, often disallowing preemptible symbols under PIC.
...
Nevertheless, I think this symbol preemptibility limitation for "s" is unfortunate. Ideally, we could retain the current "i" for immediate integer operand (after linking), and design "s" for a raw symbol name with a constant offset, ignoring symbol preemptibility. This architecture-agnostic "s" would simplify metadata section utilization and boost code portability.

kito-cheng

Just one minor comment, otherwise LGTM :)

kito-cheng · 2024-02-01T07:48:37Z

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

+; CHECK-LABEL: test:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    # var a+12 test
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    ret{{[l|q]}}


I don't see any line check with CHECK? maybe you forgot to drop after regen?

Thanks for catching this. Removed

Created using spr 1.3.4

GCC has supported a generic constraint "s" for a long time (since at least 1992), which references a symbol or label with an optional constant offset. "i" is a superset that also supports a constant integer. GCC's RISC-V port also supports a machine-specific constraint "S", which cannot be used with a preemptible symbol. (We don't bother to check preemptibility.) In PIC code, an external symbol is preemptible by default, making "S" less useful if you want to create an artificial reference for linker garbage collection, or define sections to hold symbol addresses: ``` void fun(); // error: impossible constraint in ‘asm’ for riscv64-linux-gnu-gcc -fpie/-fpic void foo() { asm(".reloc ., BFD_RELOC_NONE, %0" :: "S"(fun)); } // good even if -fpie/-fpic void foo() { asm(".reloc ., BFD_RELOC_NONE, %0" :: "s"(fun)); } ``` This patch adds support for "s". Modify https://reviews.llvm.org/D105254 ("S") to handle multi-depth GEPs (https://reviews.llvm.org/D61560).

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.

GCC has supported a generic constraint "s" for a long time (since at least 1992), which references a symbol or label with an optional constant offset. "i" is a superset that also supports a constant integer. GCC's RISC-V port also supports a machine-specific constraint "S", which cannot be used with a preemptible symbol. (We don't bother to check preemptibility.) In PIC code, an external symbol is preemptible by default, making "S" less useful if you want to create an artificial reference for linker garbage collection, or define sections to hold symbol addresses: ``` void fun(); // error: impossible constraint in ‘asm’ for riscv64-linux-gnu-gcc -fpie/-fpic void foo() { asm(".reloc ., BFD_RELOC_NONE, %0" :: "S"(fun)); } // good even if -fpie/-fpic void foo() { asm(".reloc ., BFD_RELOC_NONE, %0" :: "s"(fun)); } ``` This patch adds support for "s". Modify https://reviews.llvm.org/D105254 ("S") to handle multi-depth GEPs (https://reviews.llvm.org/D61560).

…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.

[𝘀𝗽𝗿] initial version

7e9a74f

Created using spr 1.3.4

MaskRay requested review from luismarques and kito-cheng January 31, 2024 21:39

llvmbot added clang Clang issues not falling into any other category backend:RISC-V clang:frontend Language frontend issues, e.g. anything involving "Sema" llvm:ir labels Jan 31, 2024

MaskRay requested a review from topperc January 31, 2024 21:39

MaskRay mentioned this pull request Feb 1, 2024

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

Merged

kito-cheng approved these changes Feb 1, 2024

View reviewed changes

kito-cheng mentioned this pull request Feb 1, 2024

Document constraint "s" riscv-non-isa/riscv-c-api-doc#66

Merged

remove unused CHECK

48b9be3

Created using spr 1.3.4

MaskRay merged commit 10a55ca into main Feb 1, 2024
4 of 5 checks passed

MaskRay deleted the users/MaskRay/spr/riscv-support-constraint-s branch February 1, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISCV] Support constraint "s" #80201

[RISCV] Support constraint "s" #80201

MaskRay commented Jan 31, 2024

llvmbot commented Jan 31, 2024 •

edited

MaskRay commented Jan 31, 2024

kito-cheng left a comment

kito-cheng Feb 1, 2024

MaskRay Feb 1, 2024

[RISCV] Support constraint "s" #80201

[RISCV] Support constraint "s" #80201

Conversation

MaskRay commented Jan 31, 2024

llvmbot commented Jan 31, 2024 • edited

MaskRay commented Jan 31, 2024

kito-cheng left a comment

Choose a reason for hiding this comment

kito-cheng Feb 1, 2024

Choose a reason for hiding this comment

MaskRay Feb 1, 2024

Choose a reason for hiding this comment

llvmbot commented Jan 31, 2024 •

edited