Re: [PATCH] RISC-V: Allow constraint "S" even if the symbol does not bind locally

[Kito Cheng <kito.cheng@gmail.com>]

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

The only difference is it also allows high, which is something like
%hi(sym), but I think it's harmless in the use case.

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

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.

[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 <maskray@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
>