| Message ID | 20240715153349.412657-1-cupertino.miranda@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | bpf: create modifier for mem operand for xchg and cmpxchg | expand |
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 } } */ > +
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 } } */ >> +
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) +{ + int old; + __atomic_exchange (p, new, &old, __ATOMIC_RELAXED); + return old; +} + +int bar64 (long *p, long *new) +{ + 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 } } */ +