diff mbox series

middle-end/102360 - adjust .DEFERRED_INIT expansion

Message ID 98o56o49-7r8r-s95q-83o5-n7o8ss4ppnpq@fhfr.qr
State New
Headers show
Series middle-end/102360 - adjust .DEFERRED_INIT expansion | expand

Commit Message

Richard Biener Sept. 16, 2021, 9:48 a.m. UTC
This avoids using native_interpret_type when we cannot do it with
the original type of the variable, instead use an integer type
for the initialization and side-step the size limitation of
native_interpret_int.

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress
(note the reported ICE happens on aarch64 only)

Richard.

2021-09-16  Richard Biener  <rguenther@suse.de>

	PR middle-end/102360
	* internal-fn.c (expand_DEFERRED_INIT): Make pattern-init
	of non-memory more robust.

	* g++.dg/pr102360.C: New testcase.
---
 gcc/internal-fn.c               | 24 ++++++---------
 gcc/testsuite/g++.dg/pr102360.C | 54 +++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 15 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr102360.C

Comments

Jakub Jelinek Sept. 16, 2021, 10:26 a.m. UTC | #1
On Thu, Sep 16, 2021 at 11:48:49AM +0200, Richard Biener via Gcc-patches wrote:
> 2021-09-16  Richard Biener  <rguenther@suse.de>
> 
> 	PR middle-end/102360
> 	* internal-fn.c (expand_DEFERRED_INIT): Make pattern-init
> 	of non-memory more robust.
> 
> 	* g++.dg/pr102360.C: New testcase.
> +	  if (can_native_interpret_type_p (var_type))
> +	    init = native_interpret_expr (var_type, buf, total_bytes);
> +	  else
> +	    {
> +	      tree itype = build_nonstandard_integer_type (total_bytes * 8, 1);

Shouldn't that 8 be BITS_PER_UNIT ?
I know we have tons of problems with BITS_PER_UNIT is not 8, but adding
further ones is unnecessary.

	Jakub
Richard Biener Sept. 16, 2021, 10:41 a.m. UTC | #2
On Thu, 16 Sep 2021, Jakub Jelinek wrote:

> On Thu, Sep 16, 2021 at 11:48:49AM +0200, Richard Biener via Gcc-patches wrote:
> > 2021-09-16  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR middle-end/102360
> > 	* internal-fn.c (expand_DEFERRED_INIT): Make pattern-init
> > 	of non-memory more robust.
> > 
> > 	* g++.dg/pr102360.C: New testcase.
> > +	  if (can_native_interpret_type_p (var_type))
> > +	    init = native_interpret_expr (var_type, buf, total_bytes);
> > +	  else
> > +	    {
> > +	      tree itype = build_nonstandard_integer_type (total_bytes * 8, 1);
> 
> Shouldn't that 8 be BITS_PER_UNIT ?
> I know we have tons of problems with BITS_PER_UNIT is not 8, but adding
> further ones is unnecessary.

Well, a byte is 8 bits and we do

      unsigned HOST_WIDE_INT total_bytes
        = tree_to_uhwi (TYPE_SIZE_UNIT (var_type));

      if (init_type == AUTO_INIT_PATTERN)
        {
          unsigned char *buf = (unsigned char *) xmalloc (total_bytes);
          memset (buf, INIT_PATTERN_VALUE, total_bytes);

and thus mix host and target here.  I suppose it should be instead

   unsigned HOST_WIDE_INT total_bytes
     = tree_to_uhwi (TYPE_SIZE (var_type)) / (BITS_PER_UNIT / 8);

or so...  in this light * 8 for the build_nonstandard_integer_type
use is correct, no?  If total_bytes is really _bytes_.

Richard.
Jakub Jelinek Sept. 16, 2021, 10:51 a.m. UTC | #3
On Thu, Sep 16, 2021 at 12:41:20PM +0200, Richard Biener wrote:
> On Thu, 16 Sep 2021, Jakub Jelinek wrote:
> 
> > On Thu, Sep 16, 2021 at 11:48:49AM +0200, Richard Biener via Gcc-patches wrote:
> > > 2021-09-16  Richard Biener  <rguenther@suse.de>
> > > 
> > > 	PR middle-end/102360
> > > 	* internal-fn.c (expand_DEFERRED_INIT): Make pattern-init
> > > 	of non-memory more robust.
> > > 
> > > 	* g++.dg/pr102360.C: New testcase.
> > > +	  if (can_native_interpret_type_p (var_type))
> > > +	    init = native_interpret_expr (var_type, buf, total_bytes);
> > > +	  else
> > > +	    {
> > > +	      tree itype = build_nonstandard_integer_type (total_bytes * 8, 1);
> > 
> > Shouldn't that 8 be BITS_PER_UNIT ?
> > I know we have tons of problems with BITS_PER_UNIT is not 8, but adding
> > further ones is unnecessary.
> 
> Well, a byte is 8 bits and we do
> 
>       unsigned HOST_WIDE_INT total_bytes
>         = tree_to_uhwi (TYPE_SIZE_UNIT (var_type));
> 
>       if (init_type == AUTO_INIT_PATTERN)
>         {
>           unsigned char *buf = (unsigned char *) xmalloc (total_bytes);
>           memset (buf, INIT_PATTERN_VALUE, total_bytes);
> 
> and thus mix host and target here.  I suppose it should be instead
> 
>    unsigned HOST_WIDE_INT total_bytes
>      = tree_to_uhwi (TYPE_SIZE (var_type)) / (BITS_PER_UNIT / 8);
> 
> or so...  in this light * 8 for the build_nonstandard_integer_type
> use is correct, no?  If total_bytes is really _bytes_.

Typically for the native_interpret/native_encode we punt if
BITS_PER_UNIT != 8 || CHAR_BIT != 8 because nobody had the energy
to deal with the weird platforms (especially if we have currently
none, I believe dsp16xx that had 16-bit bytes had been removed in 4.0
and c4x that had 32-bit bytes had been removed in 4.3)
- dunno if the DEFERRED_INIT etc. code has those guards or not
and it clearly documents that this code is not ready for other
configurations.
A byte is not necessarily 8 bits, that is just the most common
size for it, and TYPE_SIZE_UNIT is number of BITS_PER_UNIT bit units.

	Jakub
Richard Biener Sept. 16, 2021, 10:59 a.m. UTC | #4
On Thu, 16 Sep 2021, Jakub Jelinek wrote:

> On Thu, Sep 16, 2021 at 12:41:20PM +0200, Richard Biener wrote:
> > On Thu, 16 Sep 2021, Jakub Jelinek wrote:
> > 
> > > On Thu, Sep 16, 2021 at 11:48:49AM +0200, Richard Biener via Gcc-patches wrote:
> > > > 2021-09-16  Richard Biener  <rguenther@suse.de>
> > > > 
> > > > 	PR middle-end/102360
> > > > 	* internal-fn.c (expand_DEFERRED_INIT): Make pattern-init
> > > > 	of non-memory more robust.
> > > > 
> > > > 	* g++.dg/pr102360.C: New testcase.
> > > > +	  if (can_native_interpret_type_p (var_type))
> > > > +	    init = native_interpret_expr (var_type, buf, total_bytes);
> > > > +	  else
> > > > +	    {
> > > > +	      tree itype = build_nonstandard_integer_type (total_bytes * 8, 1);
> > > 
> > > Shouldn't that 8 be BITS_PER_UNIT ?
> > > I know we have tons of problems with BITS_PER_UNIT is not 8, but adding
> > > further ones is unnecessary.
> > 
> > Well, a byte is 8 bits and we do
> > 
> >       unsigned HOST_WIDE_INT total_bytes
> >         = tree_to_uhwi (TYPE_SIZE_UNIT (var_type));
> > 
> >       if (init_type == AUTO_INIT_PATTERN)
> >         {
> >           unsigned char *buf = (unsigned char *) xmalloc (total_bytes);
> >           memset (buf, INIT_PATTERN_VALUE, total_bytes);
> > 
> > and thus mix host and target here.  I suppose it should be instead
> > 
> >    unsigned HOST_WIDE_INT total_bytes
> >      = tree_to_uhwi (TYPE_SIZE (var_type)) / (BITS_PER_UNIT / 8);
> > 
> > or so...  in this light * 8 for the build_nonstandard_integer_type
> > use is correct, no?  If total_bytes is really _bytes_.
> 
> Typically for the native_interpret/native_encode we punt if
> BITS_PER_UNIT != 8 || CHAR_BIT != 8 because nobody had the energy
> to deal with the weird platforms (especially if we have currently
> none, I believe dsp16xx that had 16-bit bytes had been removed in 4.0
> and c4x that had 32-bit bytes had been removed in 4.3)
> - dunno if the DEFERRED_INIT etc. code has those guards or not
> and it clearly documents that this code is not ready for other
> configurations.
> A byte is not necessarily 8 bits, that is just the most common
> size for it, and TYPE_SIZE_UNIT is number of BITS_PER_UNIT bit units.

OK, I'll do s/8/BITS_PER_UNIT/ - I also see that we have
int_size_in_bytes returning TYPE_SIZE_UNIT and that TYPE_SIZE_UNIT
is documented to yeild the type size in 'bytes'.

I do believe that we should officially declare hosts with CHAR_BIT != 8
as unsupported and as you say support for targets with BITS_PER_UNIT != 8
is likely bit-rotten.

Richard.
Michael Matz Sept. 16, 2021, 3:22 p.m. UTC | #5
Hello,

On Thu, 16 Sep 2021, Richard Biener via Gcc-patches wrote:

> > Typically for the native_interpret/native_encode we punt if 
> > BITS_PER_UNIT != 8 || CHAR_BIT != 8 because nobody had the energy to 
> > deal with the weird platforms (especially if we have currently none, I 
> > believe dsp16xx that had 16-bit bytes had been removed in 4.0 and c4x 
> > that had 32-bit bytes had been removed in 4.3) - dunno if the 
> > DEFERRED_INIT etc. code has those guards or not and it clearly 
> > documents that this code is not ready for other configurations. A byte 
> > is not necessarily 8 bits, that is just the most common size for it, 
> > and TYPE_SIZE_UNIT is number of BITS_PER_UNIT bit units.
> 
> OK, I'll do s/8/BITS_PER_UNIT/ - I also see that we have 
> int_size_in_bytes returning TYPE_SIZE_UNIT and that TYPE_SIZE_UNIT is 
> documented to yeild the type size in 'bytes'.

For better or worse GCCs meaning of 'byte' is really 'unit'; I guess most 
introductions of the term 'byte' in comments and function names really 
came from either carelessness or from people who knew this fact and 
thought it obvious that 'byte' of course is the same as 'unit', but not 
the same as octet.

FWIW: (for GCC) both mean the smallest naturally addressable piece of 
memory (i.e. what you get when you increase an address by 'one'), and that 
is not necessarily 8 bit (but anything else is bit-rotten of course).

As modern use of 'byte' tends to actually mean octet, but old use of byte 
(and use in GCC) means unit, we probably should avoid the term byte 
alltogether in GCC.  But that ship has sailed :-/

> I do believe that we should officially declare hosts with CHAR_BIT != 8 
> as unsupported and as you say support for targets with BITS_PER_UNIT != 
> 8 is likely bit-rotten.

(And characters are still something else entirely, except on those couple 
platforms where chars, units and octets happen to be the same :) )  
But yes.


Ciao,
Michael.
diff mbox series

Patch

diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c
index b1283690080..842e320c31d 100644
--- a/gcc/internal-fn.c
+++ b/gcc/internal-fn.c
@@ -3045,23 +3045,17 @@  expand_DEFERRED_INIT (internal_fn, gcall *stmt)
 
       if (init_type == AUTO_INIT_PATTERN)
 	{
-	  tree alt_type = NULL_TREE;
-	  if (!can_native_interpret_type_p (var_type))
-	    {
-	      alt_type
-		= lang_hooks.types.type_for_mode (TYPE_MODE (var_type),
-						  TYPE_UNSIGNED (var_type));
-	      gcc_assert (can_native_interpret_type_p (alt_type));
-	    }
-
 	  unsigned char *buf = (unsigned char *) xmalloc (total_bytes);
 	  memset (buf, INIT_PATTERN_VALUE, total_bytes);
-	  init = native_interpret_expr (alt_type ? alt_type : var_type,
-					buf, total_bytes);
-	  gcc_assert (init);
-
-	  if (alt_type)
-	    init = build1 (VIEW_CONVERT_EXPR, var_type, init);
+	  if (can_native_interpret_type_p (var_type))
+	    init = native_interpret_expr (var_type, buf, total_bytes);
+	  else
+	    {
+	      tree itype = build_nonstandard_integer_type (total_bytes * 8, 1);
+	      wide_int w = wi::from_buffer (buf, total_bytes);
+	      init = build1 (VIEW_CONVERT_EXPR, var_type,
+			     wide_int_to_tree (itype, w));
+	    }
 	}
 
       expand_assignment (lhs, init, false);
diff --git a/gcc/testsuite/g++.dg/pr102360.C b/gcc/testsuite/g++.dg/pr102360.C
new file mode 100644
index 00000000000..fdf9e08b283
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr102360.C
@@ -0,0 +1,54 @@ 
+// { dg-do compile }
+// { dg-options "-fno-tree-dse -O1 -ftrivial-auto-var-init=pattern" }
+
+class A;
+template <typename _Tp, int m, int n> class B {
+public:
+  _Tp val[m * n];
+};
+class C {
+public:
+  C(A);
+};
+struct D {
+  D();
+  unsigned long &operator[](int);
+  unsigned long *p;
+};
+class A {
+public:
+  template <typename _Tp, int m, int n> A(const B<_Tp, m, n> &, bool);
+  int rows, cols;
+  unsigned char *data;
+  unsigned char *datastart;
+  unsigned char *dataend;
+  unsigned char *datalimit;
+  D step;
+};
+template <typename _Tp, int m, int n>
+A::A(const B<_Tp, m, n> &p1, bool)
+    : rows(m), cols(n) {
+  step[0] = cols * sizeof(_Tp);
+  datastart = data = (unsigned char *)p1.val;
+  datalimit = dataend = datastart + rows * step[0];
+}
+class F {
+public:
+  static void compute(C);
+  template <typename _Tp, int m, int n, int nm>
+  static void compute(const B<_Tp, m, n> &, B<_Tp, nm, 1> &, B<_Tp, m, nm> &,
+                      B<_Tp, n, nm> &);
+};
+D::D() {}
+unsigned long &D::operator[](int p1) { return p[p1]; }
+template <typename _Tp, int m, int n, int nm>
+void F::compute(const B<_Tp, m, n> &, B<_Tp, nm, 1> &, B<_Tp, m, nm> &,
+                B<_Tp, n, nm> &p4) {
+  A a(p4, false);
+  compute(a);
+}
+void fn1() {
+  B<double, 4, 4> b, c, e;
+  B<double, 4, 1> d;
+  F::compute(b, d, c, e);
+}