Re: [PATCH] Treat "p" in asms as addressing VOIDmode

[Andrew Pinski <pinskia@gmail.com>]

On Mon, Dec 11, 2023 at 11:46 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Jeff Law <jeffreyalaw@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