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.
Thanks,
Andrew
>
> Richard
