On Tue, Jan 30, 2024 at 11:26 PM Kito Cheng <kito.ch...@gmail.com> wrote: > > I realized there is 's' constraint which is defined in GCC generic > infra[1], and that's kinda what same as the new semantic of 'S' here, > > (define_constraint "s" > "Matches a symbolic integer constant." > (and (match_test "CONSTANT_P (op)") > (match_test "!CONST_SCALAR_INT_P (op)") > (match_test "!flag_pic || LEGITIMATE_PIC_OPERAND_P (op)"))) > > Where const, symbol_ref and label_ref is match CONSTANT_P && > !CONST_SCALAR_INT_P, > and LEGITIMATE_PIC_OPERAND_P is always 1 for RISC-V
Thanks for catching this! I read "symbolic integer constant" and skipped, but did not realized that this actually means a symbol or label reference with a constant offset. I agree that "s" should be preferred. I have jotted down some notes at https://maskray.me/blog/2024-01-30-raw-symbol-names-in-inline-assembly The condition !flag_pic || LEGITIMATE_PIC_OPERAND_P (op) highlights a key distinction in GCC's handling of symbol references: Non-PIC code (-fno-pic): The "i" and "s" constraints are freely permitted. PIC code (-fpie and -fpic): The architecture-specific LEGITIMATE_PIC_OPERAND_P(X) macro dictates whether these constraints are allowed. 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. This differentiation probably stems from historical and architectural considerations: Non-PIC code: Absolute addresses could be directly embedded in instructions like an immediate integer operand. PIC code with dynamic linking: The need for GOT indirection often requires an addressing mode different from absolute addressing and more than one instructions. 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. > The only difference is it also allows high, which is something like > %hi(sym), but I think it's harmless in the use case. I do not follow this. Do you have an example? > However I found LLVM also not work on " asm(".reloc ., BFD_RELOC_NONE, > %0" :: "S"(&ns::a[3]));", > so maybe we could consider implement 's' in LLVM? and also add some > document in riscv-c-api.md Clang does not implement the offset yet. I created https://github.com/llvm/llvm-project/pull/80201 to support "s" > And just clarify, I don't have strong prefer on using 's', I am ok > with relaxing 'S' too, > propose using 's' is because that is work fine on RISC-V gcc for long > time and no backward compatible issue, > But I guess you have this proposal may came from ClangBuiltLinux, so > 's' may not work for clang well due to backward compatible. It seems that ClangBuiltLinux can live with "i" for now:) I raised the topic due to a micro-optimization opportunity in https://github.com/protocolbuffers/protobuf/blob/1fe463ce71b6acc60b3aef65d51185e3704cac8b/src/google/protobuf/stubs/common.h#L86 and I believe metadata sections will get more used and compilers should be prepared for future uses. I'll abandon this "S" change. I can create a test-only change if you think the test coverage is useful, as we hardly have any non-rvv inline asm tests at present... > [1] > https://gcc.gnu.org/onlinedocs/gcc/Simple-Constraints.html#index-s-in-constraint > [2] > https://github.com/riscv-non-isa/riscv-c-api-doc/blob/master/riscv-c-api.md#constraints-on-operands-of-inline-assembly-statements > > On Wed, Jan 31, 2024 at 1:02 PM Fangrui Song <mask...@google.com> wrote: > > > > The constraint "S" can only be used with a symbol that binds locally, so > > the following does not work for -fpie/-fpic (GOT access is used). > > ``` > > namespace ns { extern int var, a[4]; } > > void foo() { > > asm(".pushsection .xxx,\"aw\"; .dc.a %0; .popsection" :: "S"(&ns::var)); > > asm(".reloc ., BFD_RELOC_NONE, %0" :: "S"(&ns::a[3])); > > } > > ``` > > > > This is overly restrictive, as many references like an absolute > > relocation in a writable section or a non-SHF_ALLOC section should be > > totally fine. Allow symbols that do not bind locally, similar to > > aarch64 "S" and x86-64 "Ws" (commit > > d7250100381b817114447d91fff4748526d4fb21). > > > > gcc/ChangeLog: > > > > * config/riscv/constraints.md: Relax the condition for "S". > > * doc/md.texi: Update. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.target/riscv/asm-raw-symbol.c: New test. > > --- > > gcc/config/riscv/constraints.md | 4 ++-- > > gcc/doc/md.texi | 2 +- > > gcc/testsuite/gcc.target/riscv/asm-raw-symbol.c | 17 +++++++++++++++++ > > 3 files changed, 20 insertions(+), 3 deletions(-) > > create mode 100644 gcc/testsuite/gcc.target/riscv/asm-raw-symbol.c > > > > diff --git a/gcc/config/riscv/constraints.md > > b/gcc/config/riscv/constraints.md > > index 41acaea04eb..bb012668fcb 100644 > > --- a/gcc/config/riscv/constraints.md > > +++ b/gcc/config/riscv/constraints.md > > @@ -121,8 +121,8 @@ (define_memory_constraint "A" > > (match_test "GET_CODE(XEXP(op,0)) == REG"))) > > > > (define_constraint "S" > > - "A constraint that matches an absolute symbolic address." > > - (match_operand 0 "absolute_symbolic_operand")) > > + "A symbolic reference or label reference." > > + (match_code "const,symbol_ref,label_ref")) > > > > (define_constraint "U" > > "@internal > > diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi > > index b0c61925120..c75e5bf259d 100644 > > --- a/gcc/doc/md.texi > > +++ b/gcc/doc/md.texi > > @@ -1947,7 +1947,7 @@ Integer constant that is valid as an immediate > > operand in a 64-bit @code{MOV} > > pseudo instruction > > > > @item S > > -An absolute symbolic address or a label reference > > +A symbolic reference or label reference. > > > > @item Y > > Floating point constant zero > > diff --git a/gcc/testsuite/gcc.target/riscv/asm-raw-symbol.c > > b/gcc/testsuite/gcc.target/riscv/asm-raw-symbol.c > > new file mode 100644 > > index 00000000000..eadf6d23fe1 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/riscv/asm-raw-symbol.c > > @@ -0,0 +1,17 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-fpic" } */ > > + > > +extern int var; > > + > > +void > > +func (void) > > +{ > > +label: > > + __asm__ ("@ %0" : : "S" (func)); > > + __asm__ ("@ %0" : : "S" (&var + 1)); > > + __asm__ ("@ %0" : : "S" (&&label)); > > +} > > + > > +/* { dg-final { scan-assembler "@ func" } } */ > > +/* { dg-final { scan-assembler "@ var\\+4" } } */ > > +/* { dg-final { scan-assembler "@ .L" } } */ > > -- > > 2.43.0.429.g432eaa2c6b-goog > > -- 宋方睿
