Andrew Pinski <pins...@gmail.com> writes:
> On Mon, Dec 11, 2023 at 11:46 AM Richard Sandiford
> <richard.sandif...@arm.com> wrote:
>>
>> Jeff Law <jeffreya...@gmail.com> writes:
>> > On 11/27/23 05:12, Richard Sandiford wrote:
>> >> check_asm_operands was inconsistent about how it handled "p" after
>> >> RA compared to before RA. Before RA it tested the address with a
>> >> void (unknown) memory mode:
>> >>
>> >> case CT_ADDRESS:
>> >> /* Every address operand can be reloaded to fit. */
>> >> result = result || address_operand (op, VOIDmode);
>> >> break;
>> >>
>> >> After RA it deferred to constrain_operands, which used the mode
>> >> of the operand:
>> >>
>> >> if ((GET_MODE (op) == VOIDmode
>> >> || SCALAR_INT_MODE_P (GET_MODE (op)))
>> >> && (strict <= 0
>> >> || (strict_memory_address_p
>> >> (recog_data.operand_mode[opno], op))))
>> >> win = true;
>> >>
>> >> Using the mode of the operand matches reload's behaviour:
>> >>
>> >> else if (insn_extra_address_constraint
>> >> (lookup_constraint (constraints[i])))
>> >> {
>> >> address_operand_reloaded[i]
>> >> = find_reloads_address (recog_data.operand_mode[i], (rtx*) 0,
>> >> recog_data.operand[i],
>> >> recog_data.operand_loc[i],
>> >> i, operand_type[i], ind_levels, insn);
>> >>
>> >> It allowed the special predicate address_operand to be used, with the
>> >> mode of the operand being the mode of the addressed memory, rather than
>> >> the mode of the address itself. For example, vax has:
>> >>
>> >> (define_insn "*movaddr<mode>"
>> >> [(set (match_operand:SI 0 "nonimmediate_operand" "=g")
>> >> (match_operand:VAXfp 1 "address_operand" "p"))
>> >> (clobber (reg:CC VAX_PSL_REGNUM))]
>> >> "reload_completed"
>> >> "mova<VAXfp:fsfx> %a1,%0")
>> >>
>> >> where operand 1 is an SImode expression that can address memory of
>> >> mode VAXfp. GET_MODE (recog_data.operand[1]) is SImode (or VOIDmode),
>> >> but recog_data.operand_mode[1] is <VAXfp:MODE>mode.
>> >>
>> >> But AFAICT, ira and lra (like pre-reload check_asm_operands) do not
>> >> do this, and instead pass VOIDmode. So I think this traditional use
>> >> of address_operand is effectively an old-reload-only feature.
>> >>
>> >> And it seems like no modern port cares. I think ports have generally
>> >> moved to using different address constraints instead, rather than
>> >> relying on "p" with different operand modes. Target-specific address
>> >> constraints post-date the code above.
>> >>
>> >> The big advantage of using different constraints is that it works
>> >> for asms too. And that (to finally get to the point) is the problem
>> >> fixed in this patch. For the aarch64 test:
>> >>
>> >> void f(char *p) { asm("prfm pldl1keep, %a0\n" :: "p" (p + 6)); }
>> >>
>> >> everything up to and including RA required the operand to be a
>> >> valid VOIDmode address. But post-RA check_asm_operands and
>> >> constrain_operands instead required it to be valid for
>> >> recog_data.operand_mode[0]. Since asms have no syntax for
>> >> specifying an operand mode that's separate from the operand itself,
>> >> operand_mode[0] is simply Pmode (i.e. DImode).
>> >>
>> >> This meant that we required one mode before RA and a different mode
>> >> after RA. On AArch64, VOIDmode is treated as a wildcard and so has a
>> >> more conservative/restricted range than DImode. So if a post-RA pass
>> >> tried to form a new address, it would use a laxer condition than the
>> >> pre-RA passes.
>> > This was initially a bit counter-intuitive, my first reaction was that a
>> > wildcard mode is more general. And that's true, but it necessarily
>> > means the addresses accepted are more restrictive because any mode is
>> > allowed.
>>
>> Right. I should probably have a conservative, common subset.
>>
>> >> This happened with the late-combine pass that I posted in October:
>> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-October/634166.html
>> >> which in turn triggered an error from aarch64_print_operand_address.
>> >>
>> >> This patch takes the (hopefully) conservative fix of using VOIDmode for
>> >> asms but continuing to use the operand mode for .md insns, so as not
>> >> to break ports that still use reload.
>> > Sadly I didn't get as far as I would have liked in removing reload,
>> > though we did get a handful of ports converted this cycle
>> >
>> >>
>> >> Fixing this made me realise that recog_level2 was doing duplicate
>> >> work for asms after RA.
>> >>
>> >> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
>> >>
>> >> Richard
>> >>
>> >>
>> >> gcc/
>> >> * recog.cc (constrain_operands): Pass VOIDmode to
>> >> strict_memory_address_p for 'p' constraints in asms.
>> >> * rtl-ssa/changes.cc (recog_level2): Skip redundant
>> >> constrain_operands
>> >> for asms.
>> >>
>> >> gcc/testsuite/
>> >> * gcc.target/aarch64/prfm_imm_offset_2.c: New test.
>> > It all seems a bit hackish. I don't think ports have had much success
>> > using 'p' through the decades. I think I generally ended up having to
>> > go with distinct constraints rather than relying on 'p'.
>> >
>> > OK for the trunk, but ewww.
>>
>> Thanks, pushed. And yeah, eww is fair. I'd be happy for this to become
>> an unconditional VOIDmode once reload is removed.
>
>
> The testcase prfm_imm_offset_2.c fails with a compile error:
> /home/apinski/src/upstream-full-cross/gcc/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c:
> In function 'f':
> /home/apinski/src/upstream-full-cross/gcc/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c:1:46:
> error: expected ')' before ':' token
>
> Most likely you need to add `/* { dg-options "" } */` to the beginning
> of the file so it does not compile with `-ansi -pedantic-errors`
> options which are default for the testsuite.
Yeah, sorry. I fixed this at least twice in my tree, but I was having
to carry the patch around in multiple branches, and must have cherry-picked
from an unfixed branch.
Here's what I pushed. Sorry for the breakage.
Richard
gcc/testsuite/
* gcc.target/aarch64/prfm_imm_offset_2.c: Add dg-options.
---
gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c
b/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c
index 2dd695157f2..04e3fb72c45 100644
--- a/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c
+++ b/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c
@@ -1 +1,2 @@
+/* { dg-options "-O2" } */
void f(char *p) { asm("prfm pldl1keep, %a0\n" :: "p" (p + 6)); }
--
2.25.1
