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

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

[Richard Sandiford]

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

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)); }
-- 
2.25.1

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

[Richard Sandiford]

Ping

---

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

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)); }

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

[Jeff Law]

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

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

[Maciej W. Rozycki]

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

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

[Richard Sandiford]

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

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

[Andrew Pinski]

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

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

[Richard Sandiford]

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)); }
-- 
2.25.1

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

[Maciej W. Rozycki]

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