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.
Richard
