| Message ID | mptsf4rxun8.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Treat "p" in asms as addressing VOIDmode | expand |
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. > > 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. jeff
On Mon, 11 Dec 2023, Jeff Law wrote: > > 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 The VAX port isn't ready for LRA yet as not only LRA produces notably worse RISC-like code ignoring all the architecture's address mode features (unenthusiatically acceptable), but it causes testsuite regressions of the ICE kind (unacceptable) as well. I did hope to at least start work on it in this release cycle, but there has been this outstanding issue of broken exception unwinding, which makes C++ unusuable except for odd cases such as with GCC itself where exceptions are not used. This unwinder issue obviously has to take precedence as it cripples the usability of code produced by the compiler even for developer's use, e.g. native VAX/GDB is mostly broken and even VAX/gdbserver quits with a crash. I can try and see if I can find some time over the festive period to move the VAX port forward in either respect. Maciej
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. Richard
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
Andrew Pinski <pinskia@gmail.com> writes: > 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. 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)); }
On Mon, 11 Dec 2023, Richard Sandiford wrote: > > 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. Hmm, LRA seems unable currently to work with indexed address modes, such as with these address load machine instructions: movaq 0x12345678[%r1],%r2 movaq (%r0)[%r1],%r2 movaq 0x12345678(%r0)[%r1],%r2 movaq *0x12345678[%r1],%r2 movaq *(%r0)[%r1],%r2 movaq *0x12345678(%r0)[%r1],%r2 (where R1 is scaled according to the width of data the address refers to before adding to the direct or indirect address component worked out from base+displacement, by 8 in this example, suitably for DImode or DFmode) so who knows what we'll end up with once the VAX port has been converted. Maciej
diff --git a/gcc/recog.cc b/gcc/recog.cc index eaab79c25d7..bff7be1aec1 100644 --- a/gcc/recog.cc +++ b/gcc/recog.cc @@ -3191,13 +3191,17 @@ constrain_operands (int strict, alternative_mask alternatives) strictly valid, i.e., that all pseudos requiring hard regs have gotten them. We also want to make sure we have a valid mode. */ - 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; - break; + { + auto mem_mode = (recog_data.is_asm + ? VOIDmode + : recog_data.operand_mode[opno]); + if ((GET_MODE (op) == VOIDmode + || SCALAR_INT_MODE_P (GET_MODE (op))) + && (strict <= 0 + || strict_memory_address_p (mem_mode, op))) + win = true; + break; + } /* No need to check general_operand again; it was done in insn-recog.cc. Well, except that reload diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc index 2f2d12d5f30..443d0575df5 100644 --- a/gcc/rtl-ssa/changes.cc +++ b/gcc/rtl-ssa/changes.cc @@ -986,8 +986,10 @@ recog_level2 (insn_change &change, add_regno_clobber_fn add_regno_clobber) pat = newpat; } + // check_asm_operands checks the constraints after RA, so we don't + // need to do it again. INSN_CODE (rtl) = icode; - if (reload_completed) + if (reload_completed && !asm_p) { extract_insn (rtl); if (!constrain_operands (1, get_preferred_alternatives (rtl))) diff --git a/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c b/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c new file mode 100644 index 00000000000..04e3fb72c45 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c @@ -0,0 +1,2 @@ +/* { dg-options "-O2" } */ +void f(char *p) { asm("prfm pldl1keep, %a0\n" :: "p" (p + 6)); }
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 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. 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. --- gcc/recog.cc | 18 +++++++++++------- gcc/rtl-ssa/changes.cc | 4 +++- .../gcc.target/aarch64/prfm_imm_offset_2.c | 2 ++ 3 files changed, 16 insertions(+), 8 deletions(-) create mode 100644 gcc/testsuite/gcc.target/aarch64/prfm_imm_offset_2.c