middle-end/104786 - ICE with asm and VLA

  The following fixes an ICE observed with a MEM_REF allows_mem asm
operand.  There's code expecting INDIRECT_REFs that are now never
going to appear.  The following simply treats all tcc_reference
operands the same.

A more conservative approach would be to use INDIRECT_REF || MEM_REF
which would fix this particular testcase, but all non-register typed
kinds of reference trees could appear here and I'm not sure why
former INDIRECT_REFs should be OK to go this path but not say
a.b as we also handle plain DECL_P here albeit with some extra
constraints.  For the VLA case in the PR another spot to adjust
would be || TREE_ADDRESSABLE (type) noting that we also cannot
create a temporary for not constant-sized types.

That said - INDIRECT_REF catched my eye here, this might have
caused quite some extra copies eventually so I did want to fix
that.

Btw, it seems to me the DECL_P case disallowing promoted variables
should only apply to register types which should be SSA names
here unless TREE_ADDRESSABLE in which case we let them through
anyway.  With the same reasoning a generic REFERENCE_CLASS_P
should be OK, not needing such special-casing.

Bootstrap and regtest running on x86_64-unknown-linux-gnu.

OK for trunk?

Thanks,
Richard.

2022-03-09  Richard Biener  <rguenther@suse.de>

	PR middle-end/104786
	* cfgexpand.cc (expand_asm_stmt): Specialize REFERENCE_CLASS_P
	operands rather than INDIRECT_REFs.

	* gcc.dg/pr104786.c: New testcase.
---
 gcc/cfgexpand.cc                | 2 +-
 gcc/testsuite/gcc.dg/pr104786.c | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr104786.c
  

On Wed, Mar 09, 2022 at 11:07:21AM +0100, Richard Biener wrote:
> The following fixes an ICE observed with a MEM_REF allows_mem asm
> operand.  There's code expecting INDIRECT_REFs that are now never
> going to appear.  The following simply treats all tcc_reference
> operands the same.

The INDIRECT_REF in there seems to be at least from 1995, but my limited
understanding of what it wants to catch is operands that provably must
live in memory.  INDIRECT_REF (assuming *&*& is folded already) is
such a case, MEM_REF could be but not always (there is the case of
MEM_REF on &decl without TREE_ADDRESSABLE) but usually is,
other handled_component_p depend on what their first operand is etc.

I think there are two cases, one is an optimization (the && allows_mem
case) where we don't want create a temporary (especially a huge one)
if it clearly has to live in memory.  Example where we mess this up:
struct T { char b[1024]; };
struct S { int a; struct T b; } s;

void
foo (void)
{
  asm volatile ("# %0" : : "rm" (s.b));
}
where we create 1024 bytes long temporary on the stack, copy s.b to it
and that is the operand live in memory.
So perhaps for the allows_mem && case we could handle that way
all BLKmode types (BLKmode must live in memory, no?) and otherwise
look through all handled_component_p's and determine if the base
will be in memory (addressable decl, MEM_REF except for ADDR of
non-addressable decl, INDIRECT_REF, what else?).

And another case is where it is not an optimization, where we
just can't ever create a temporary.
One of those examples is
		|| TREE_ADDRESSABLE (type)
the if already has, I guess non-constant length (ok, SVE is ok)
would be another.
assign_temp tests !poly_int_tree_p (TYPE_SIZE_UNIT (type), &size),
so perhaps add || !tree_fits_poly_int64_p (TYPE_SIZE_UNIT (type)) ?

	Jakub
  

On Wed, 9 Mar 2022, Jakub Jelinek wrote:

> On Wed, Mar 09, 2022 at 11:07:21AM +0100, Richard Biener wrote:
> > The following fixes an ICE observed with a MEM_REF allows_mem asm
> > operand.  There's code expecting INDIRECT_REFs that are now never
> > going to appear.  The following simply treats all tcc_reference
> > operands the same.
> 
> The INDIRECT_REF in there seems to be at least from 1995, but my limited
> understanding of what it wants to catch is operands that provably must
> live in memory.  INDIRECT_REF (assuming *&*& is folded already) is
> such a case, MEM_REF could be but not always (there is the case of
> MEM_REF on &decl without TREE_ADDRESSABLE) but usually is,
> other handled_component_p depend on what their first operand is etc.
> 
> I think there are two cases, one is an optimization (the && allows_mem
> case) where we don't want create a temporary (especially a huge one)
> if it clearly has to live in memory.  Example where we mess this up:
> struct T { char b[1024]; };
> struct S { int a; struct T b; } s;
> 
> void
> foo (void)
> {
>   asm volatile ("# %0" : : "rm" (s.b));
> }
> where we create 1024 bytes long temporary on the stack, copy s.b to it
> and that is the operand live in memory.
> So perhaps for the allows_mem && case we could handle that way
> all BLKmode types (BLKmode must live in memory, no?) and otherwise
> look through all handled_component_p's and determine if the base
> will be in memory (addressable decl, MEM_REF except for ADDR of
> non-addressable decl, INDIRECT_REF, what else?).

Well, in theory you could have a MEM_REF<struct T> (&int_decl)
with a BLKmode struct T but SImode int_decl.  So looking at the
mode isn't good enough I think.  Note that the DECL_P case
does simply allow a non-MEM_P DECL_RTL when allows_mem and
only disallows the case of a REG with a different mode than
the type (but if it say a CONCAT it doesn't care).  That was
my reasoning that any additional restrictions are not necessary.
Of course we might run into

          if (! allows_reg && !MEM_P (op))
            {
              error_at (locus, "output number %d not directly 
addressable", i);
              error_seen = true;
            }

then, so when !allows_reg it might be better to go the
temporary copy way.  But then I fail to see how !allows_mem
is OK in that case ...

> And another case is where it is not an optimization, where we
> just can't ever create a temporary.
> One of those examples is
> 		|| TREE_ADDRESSABLE (type)
> the if already has, I guess non-constant length (ok, SVE is ok)
> would be another.
> assign_temp tests !poly_int_tree_p (TYPE_SIZE_UNIT (type), &size),
> so perhaps add || !tree_fits_poly_int64_p (TYPE_SIZE_UNIT (type)) ?

Yes, and that's good enough for the testcase at hand.  assign_temp
also handles some cases through max_int_size_in_bytes.  I'm going
to leave the optimization above for a followup and will test the
following for now.

Richard.

From 90066f8dc0353103b9e7276b7a3e18e925509ded Mon Sep 17 00:00:00 2001
From: Richard Biener <rguenther@suse.de>
Date: Wed, 9 Mar 2022 10:55:49 +0100
Subject: [PATCH] middle-end/104786 - ICE with asm and VLA
To: gcc-patches@gcc.gnu.org

The following fixes an ICE observed with a MEM_REF allows_mem asm
operand referencing a VLA.  The following makes sure to not attempt
to go the temporary creation way when we cannot.

2022-03-09  Richard Biener  <rguenther@suse.de>

	PR middle-end/104786
	* cfgexpand.cc (expand_asm_stmt): Do not generate a copy
	for VLAs without an upper size bound.

	* gcc.dg/pr104786.c: New testcase.
---
 gcc/cfgexpand.cc                | 4 +++-
 gcc/testsuite/gcc.dg/pr104786.c | 8 ++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr104786.c

diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
index 87536ec7ccd..271bbfb8fe9 100644
--- a/gcc/cfgexpand.cc
+++ b/gcc/cfgexpand.cc
@@ -3297,7 +3297,9 @@ expand_asm_stmt (gasm *stmt)
 		    && GET_MODE (DECL_RTL (val)) != TYPE_MODE (type)))
 	  || ! allows_reg
 	  || is_inout
-	  || TREE_ADDRESSABLE (type))
+	  || TREE_ADDRESSABLE (type)
+	  || (!tree_fits_poly_int64_p (type)
+	      && !known_size_p (max_int_size_in_bytes (type))))
 	{
 	  op = expand_expr (val, NULL_RTX, VOIDmode,
 			    !allows_reg ? EXPAND_MEMORY : EXPAND_WRITE);
diff --git a/gcc/testsuite/gcc.dg/pr104786.c b/gcc/testsuite/gcc.dg/pr104786.c
new file mode 100644
index 00000000000..3076d236d21
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr104786.c
@@ -0,0 +1,8 @@
+/* { dg-do compile } */
+/* { dg-options "-std=gnu90" } */
+
+void h(void *di, int num)
+{
+  char (*t)[num] = di;
+  __asm__ ("" : "=X"( *t));
+}
  

On Wed, Mar 09, 2022 at 12:30:08PM +0100, Richard Biener wrote:
> The following fixes an ICE observed with a MEM_REF allows_mem asm
> operand referencing a VLA.  The following makes sure to not attempt
> to go the temporary creation way when we cannot.
> 
> 2022-03-09  Richard Biener  <rguenther@suse.de>
> 
> 	PR middle-end/104786
> 	* cfgexpand.cc (expand_asm_stmt): Do not generate a copy
> 	for VLAs without an upper size bound.
> 
> 	* gcc.dg/pr104786.c: New testcase.

LGTM.

	Jakub