diff mbox

Fix PR48183, NEON ICE in emit-rtl.c:immed_double_const() under -g

Message ID g4fwq6qk61.fsf@linaro.org
State New
Headers show

Commit Message

Richard Sandiford March 29, 2011, 10:45 a.m. UTC 
Julian Brown <julian@codesourcery.com> writes:
> On Thu, 24 Mar 2011 10:57:06 +0000
> Richard Sandiford <richard.sandiford@linaro.org> wrote:
>
>> Chung-Lin Tang <cltang@codesourcery.com> writes:
>> > PR48183 is a case where ARM NEON instrinsics, under -O -g, produce
>> > debug insns that tries to expand OImode (32-byte integer) zero
>> > constants, much too large to represent as two HOST_WIDE_INTs; as
>> > the internals manual indicates, such large constants are not
>> > supported in general, and ICEs on the GET_MODE_BITSIZE(mode) ==
>> > 2*HOST_BITS_PER_WIDE_INT assertion.
>> >
>> > This patch allows the cases where the large integer constant is
>> > still representable using a single CONST_INT, such as zero(0).
>> > Bootstrapped and tested on i686 and x86_64, cross-tested on ARM,
>> > all without regressions. Okay for trunk?
>> >
>> > Thanks,
>> > Chung-Lin
>> >
>> > 2011-03-20  Chung-Lin Tang  <cltang@codesourcery.com>
>> >
>> > 	* emit-rtl.c (immed_double_const): Allow wider than
>> > 	2*HOST_BITS_PER_WIDE_INT mode constants when they are
>> > 	representable as a single const_int RTX.
>> 
>> I realise this might be seen as a good expedient fix, but it makes
>> me a bit uneasy.  Not a very constructive rationale, sorry.
>
> FWIW I also had a "fix" for this issue, which is equivalent to
> Chung-Lin's patch apart from only allowing constant-zero (attached).
> That's not really a vote from me for this approach, but maybe limiting
> the extent to which we pretend to support wide-integer constants like
> this is sensible, if we do go that way.

So that each suggestion has a patch, here's mine.  I've snipped the
regenerated file from the testcase, but to get a flavour, the changes
are like this:

@@ -6099,63 +6099,72 @@ vtbl1_p8 (poly8x8_t __a, uint8x8_t __b)
 __extension__ static __inline int8x8_t __attribute__ ((__always_inline__))
 vtbl2_s8 (int8x8x2_t __a, int8x8_t __b)
 {
-  union { int8x8x2_t __i; __builtin_neon_ti __o; } __au = { __a };
+  union { int8x8x2_t __i; __builtin_neon_ti __o; } __au;
+  __au.__i = __a;
   return (int8x8_t)__builtin_neon_vtbl2v8qi (__au.__o, __b);
 }

Of course, Chung-Lin's testcase should be included whichever way we go.

Richard


gcc/
	* config/arm/neon-gen.ml (params): Separate the union initialisation
	from the union itself.  Return a list of statements.
	(print_variant): Update call accordingly.  Add the returned statements
	before the ones returned by "return".
	* config/arm/arm_neon.h: Regenerate.

Comments

Julian Brown March 30, 2011, 11:20 a.m. UTC | #1 
On Tue, 29 Mar 2011 11:45:58 +0100
Richard Sandiford <richard.sandiford@linaro.org> wrote:

> Julian Brown <julian@codesourcery.com> writes:
> > On Thu, 24 Mar 2011 10:57:06 +0000
> > Richard Sandiford <richard.sandiford@linaro.org> wrote:
> >
> >> Chung-Lin Tang <cltang@codesourcery.com> writes:
> >> > PR48183 is a case where ARM NEON instrinsics, under -O -g,
> >> > produce debug insns that tries to expand OImode (32-byte
> >> > integer) zero constants, much too large to represent as two
> >> > HOST_WIDE_INTs; as the internals manual indicates, such large
> >> > constants are not supported in general, and ICEs on the
> >> > GET_MODE_BITSIZE(mode) == 2*HOST_BITS_PER_WIDE_INT assertion.
> >> >
> >> > This patch allows the cases where the large integer constant is
> >> > still representable using a single CONST_INT, such as zero(0).
> >> > Bootstrapped and tested on i686 and x86_64, cross-tested on ARM,
> >> > all without regressions. Okay for trunk?
> >> >
> >> > Thanks,
> >> > Chung-Lin
> >> >
> >> > 2011-03-20  Chung-Lin Tang  <cltang@codesourcery.com>
> >> >
> >> > 	* emit-rtl.c (immed_double_const): Allow wider than
> >> > 	2*HOST_BITS_PER_WIDE_INT mode constants when they are
> >> > 	representable as a single const_int RTX.
> >> 
> >> I realise this might be seen as a good expedient fix, but it makes
> >> me a bit uneasy.  Not a very constructive rationale, sorry.
> >
> > FWIW I also had a "fix" for this issue, which is equivalent to
> > Chung-Lin's patch apart from only allowing constant-zero (attached).
> > That's not really a vote from me for this approach, but maybe
> > limiting the extent to which we pretend to support wide-integer
> > constants like this is sensible, if we do go that way.
> 
> So that each suggestion has a patch, here's mine.  I've snipped the
> regenerated file from the testcase, but to get a flavour, the changes
> are like this:

The O'Caml changes look fine, FWIW (I can't officially approve them, of
course) -- and I certainly don't have a problem with avoiding my or
Chung-Lin's hacks, though of course it'd be nice to get rid of the
wide-integer constant "problem" properly at some point.

Cheers,

Julian
diff mbox

Patch

Index: gcc/config/arm/neon-gen.ml
===================================================================
--- gcc/config/arm/neon-gen.ml	2009-11-11 15:08:40.000000000 +0000
+++ gcc/config/arm/neon-gen.ml	2011-03-29 11:37:52.000000000 +0100
@@ -165,12 +165,14 @@  let rec element_type ctype =
 
 let params return_by_ptr ps =
   let pdecls = ref [] in
+  let pstmts = ref [] in
   let ptype t p =
     match t with
       T_arrayof (num, elts) ->
-        let uname = union_string num elts (p ^ "u") in
-        let decl = Printf.sprintf "%s = { %s };" uname p in
+        let decl = union_string num elts (p ^ "u") ^ ";" in
+        let assignment = p ^ "u.__i = " ^ p ^ ";" in
         pdecls := decl :: !pdecls;
+        pstmts := assignment :: !pstmts;
         p ^ "u.__o"
     | _ -> add_cast t p in
   let plist = match ps with
@@ -183,10 +185,11 @@  let params return_by_ptr ps =
   match ps with
     Arity0 ret | Arity1 (ret, _) | Arity2 (ret, _, _) | Arity3 (ret, _, _, _)
   | Arity4 (ret, _, _, _, _) ->
+      !pdecls, !pstmts,
       if return_by_ptr then
-        !pdecls, add_cast (T_ptrto (element_type ret)) "&__rv.val[0]" :: plist
+        add_cast (T_ptrto (element_type ret)) "&__rv.val[0]" :: plist
       else
-        !pdecls, plist
+        plist
 
 let modify_params features plist =
   let is_flipped =
@@ -243,14 +246,14 @@  let print_variant opcode features shape 
   let bits = infoword_value elttype features in
   let modesuf = mode_suffix elttype shape in
   let return_by_ptr = return_by_ptr features in
-  let pdecls, paramlist = params return_by_ptr ctype in
+  let pdecls, pstmts, paramlist = params return_by_ptr ctype in
   let paramlist' = modify_params features paramlist in
   let paramlist'' = extra_word shape features paramlist' bits in
   let parstr = String.concat ", " paramlist'' in
   let builtin = Printf.sprintf "__builtin_neon_%s%s (%s)"
                   (builtin_name features name) modesuf parstr in
-  let rdecls, stmts = return ctype return_by_ptr builtin in
-  let body = pdecls @ rdecls @ stmts
+  let rdecls, rstmts = return ctype return_by_ptr builtin in
+  let body = pdecls @ rdecls @ pstmts @ rstmts
   and fnname = (intrinsic_name name) ^ "_" ^ (string_of_elt elttype) in
   print_function ctype fnname body