Fixed and pushed!
Thanks,
Cupertino
David Faust writes:
> On 7/15/24 08:33, Cupertino Miranda wrote:
>> Both xchg and cmpxchg instructions, in the pseudo-C dialect, do not
>> expect their memory address operand to be surrounded by parentheses.
>> For example, it should be output as "w0 =cmpxchg32_32(r8+8,w0,w2)"
>> instead of "w0 =cmpxchg32_32((r8+8),w0,w2)".
>
> After some brief searching, looks like these extra parens in [cmp]xchg*
> are an unintended consequence of:
>
> bb5f619a938 bpf: fix printing of memory operands in pseudoc asm dialect
>
>>
>> This patch implements an operand modifier 'M' which marks the
>> instruction templates that do not expect the parentheses, and adds it do
>> xchg and cmpxchg templates.
>
> One very minor nit below, otherwise LGTM. Please apply.
> Thanks for the fix!
>
>>
>> gcc/ChangeLog:
>> * config/bpf/atomic.md (atomic_compare_and_swap,
>> atomic_exchange): Add operand modifier %M to the first
>> operand.
>> * config/bpf/bpf.cc (no_parentheses_mem_operand): Create
>> variable.
>> (bpf_print_operand): Set no_parentheses_mem_operand variable if
>> %M operand is used.
>> (bpf_print_operand_address): Conditionally output parentheses.
>>
>> gcc/testsuite/ChangeLog:
>> * gcc.target/bpf/pseudoc-atomic-memaddr-op.c: Add test.
>> ---
>> gcc/config/bpf/atomic.md | 4 +--
>> gcc/config/bpf/bpf.cc | 20 ++++++++---
>> .../bpf/pseudoc-atomic-memaddr-op.c | 36 +++++++++++++++++++
>> 3 files changed, 54 insertions(+), 6 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.target/bpf/pseudoc-atomic-memaddr-op.c
>>
>> diff --git a/gcc/config/bpf/atomic.md b/gcc/config/bpf/atomic.md
>> index 65bd5f266a1..be4511bb51b 100644
>> --- a/gcc/config/bpf/atomic.md
>> +++ b/gcc/config/bpf/atomic.md
>> @@ -129,7 +129,7 @@
>> (set (match_dup 1)
>> (match_operand:AMO 2 "nonmemory_operand" "0"))]
>> "bpf_has_v3_atomics"
>> - "{axchg<msuffix>\t%1,%0|%w0 = xchg<pcaxsuffix>(%1, %w0)}")
>> + "{axchg<msuffix>\t%1,%0|%w0 = xchg<pcaxsuffix>(%M1, %w0)}")
>>
>> ;; The eBPF atomic-compare-and-exchange instruction has the form
>> ;; acmp [%dst+offset], %src
>> @@ -182,4 +182,4 @@
>> (match_operand:AMO 3 "register_operand")] ;; desired
>> UNSPEC_ACMP))]
>> "bpf_has_v3_atomics"
>> - "{acmp<msuffix>\t%1,%3|%w0 = cmpxchg<pcaxsuffix>(%1, %w0, %w3)}")
>> + "{acmp<msuffix>\t%1,%3|%w0 = cmpxchg<pcaxsuffix>(%M1, %w0, %w3)}")
>> diff --git a/gcc/config/bpf/bpf.cc b/gcc/config/bpf/bpf.cc
>> index 687e7ba49c1..a6b6e20731c 100644
>> --- a/gcc/config/bpf/bpf.cc
>> +++ b/gcc/config/bpf/bpf.cc
>> @@ -835,6 +835,11 @@ bpf_print_register (FILE *file, rtx op, int code)
>> }
>> }
>>
>> +/* Variable defined to implement 'M' operand modifier for the special cases
>> + where the parentheses should not be printed surrounding a memory address
>> + operand. */
>> +static bool no_parentheses_mem_operand;
>> +
>> /* Print an instruction operand. This function is called in the macro
>> PRINT_OPERAND defined in bpf.h */
>>
>> @@ -847,6 +852,7 @@ bpf_print_operand (FILE *file, rtx op, int code)
>> bpf_print_register (file, op, code);
>> break;
>> case MEM:
>> + no_parentheses_mem_operand = (code == 'M');
>> output_address (GET_MODE (op), XEXP (op, 0));
>> break;
>> case CONST_DOUBLE:
>> @@ -891,6 +897,9 @@ bpf_print_operand (FILE *file, rtx op, int code)
>> }
>> }
>>
>> +#define PAREN_OPEN (asm_dialect == ASM_NORMAL ? "[" :
>> no_parentheses_mem_operand ? "" : "(")
>> +#define PAREN_CLOSE (asm_dialect == ASM_NORMAL ? "]" :
>> no_parentheses_mem_operand ? "" : ")")
>> +
>> /* Print an operand which is an address. This function should handle
>> any legit address, as accepted by bpf_legitimate_address_p, and
>> also addresses that are valid in CALL instructions.
>> @@ -904,9 +913,9 @@ bpf_print_operand_address (FILE *file, rtx addr)
>> switch (GET_CODE (addr))
>> {
>> case REG:
>> - fprintf (file, asm_dialect == ASM_NORMAL ? "[" : "(");
>> + fprintf (file, "%s", PAREN_OPEN);
>> bpf_print_register (file, addr, 0);
>> - fprintf (file, asm_dialect == ASM_NORMAL ? "+0]" : "+0)");
>> + fprintf (file, "+0%s", PAREN_CLOSE);
>> break;
>> case PLUS:
>> {
>> @@ -923,14 +932,14 @@ bpf_print_operand_address (FILE *file, rtx addr)
>> || (GET_CODE (op1) == UNSPEC
>> && XINT (op1, 1) == UNSPEC_CORE_RELOC)))
>> {
>> - fprintf (file, asm_dialect == ASM_NORMAL ? "[" : "(");
>> + fprintf (file, "%s", PAREN_OPEN);
>> bpf_print_register (file, op0, 0);
>> fprintf (file, "+");
>> if (GET_CODE (op1) == UNSPEC)
>> output_addr_const (file, XVECEXP (op1, 0, 0));
>> else
>> output_addr_const (file, op1);
>> - fprintf (file, asm_dialect == ASM_NORMAL ? "]" : ")");
>> + fprintf (file, "%s", PAREN_CLOSE);
>> }
>> else
>> fatal_insn ("invalid address in operand", addr);
>> @@ -948,6 +957,9 @@ bpf_print_operand_address (FILE *file, rtx addr)
>> }
>> }
>>
>> +#undef PAREN_OPEN
>> +#undef PAREN_CLOSE
>> +
>> /* Add a BPF builtin function with NAME, CODE and TYPE. Return
>> the function decl or NULL_TREE if the builtin was not added. */
>>
>> diff --git a/gcc/testsuite/gcc.target/bpf/pseudoc-atomic-memaddr-op.c
>> b/gcc/testsuite/gcc.target/bpf/pseudoc-atomic-memaddr-op.c
>> new file mode 100644
>> index 00000000000..758faf04cc2
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/bpf/pseudoc-atomic-memaddr-op.c
>> @@ -0,0 +1,36 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-mv3-atomics -O2 -masm=pseudoc" } */
>> +
>> +int
>> +foo (int *p, int *expected, int desired)
>> +{
>> + return __atomic_compare_exchange (p, expected, &desired, 0,
>> + __ATOMIC_ACQUIRE, __ATOMIC_RELAXED);
>> +}
>> +
>> +int
>> +foo64 (long *p, long *expected, long desired)
>> +{
>> + return __atomic_compare_exchange (p, expected, &desired, 0,
>> + __ATOMIC_ACQUIRE, __ATOMIC_RELAXED);
>> +}
>> +
>> +int bar (int *p, int *new)
>
> nit: style - return type should be on its own line
>
>> +{
>> + int old;
>> + __atomic_exchange (p, new, &old, __ATOMIC_RELAXED);
>> + return old;
>> +}
>> +
>> +int bar64 (long *p, long *new)
>
> same here
>
>> +{
>> + long old;
>> + __atomic_exchange (p, new, &old, __ATOMIC_SEQ_CST);
>> + return old;
>> +}
>> +
>> +/* { dg-final { scan-assembler "r. = cmpxchg_64\\(r.\\+0, r., r.\\)" } } */
>> +/* { dg-final { scan-assembler "w. = cmpxchg32_32\\(r.\\+0, w., w.\\)" } }
>> */
>> +/* { dg-final { scan-assembler-times "w. = xchg32_32\\(r.\\+0, w.\\)" 1 } }
>> */
>> +/* { dg-final { scan-assembler-times "r. = xchg_64\\(r.\\+0, r.\\)" 1 } } */
>> +
