rfc: constant pool and floats

rfc: constant pool and floats

[Aldy Hernandez]

Hi folks.

The following testcase is failing on powerpc-linux as well as the embedded
variants.

FAIL: tmpdir-gcc.dg-struct-layout-1/t001 c_compat_x_tst.o compile

I have narrowed the problem down to:

	typedef float Tal1float __attribute__ ((aligned (2)));
	struct S38 { Tal1float a; };
	struct S38 s38;
	void test38 (void) { s38.a = 91611.765625; }

Where 2 would be an unaligned access to a float.

store_bit_field() changes the floating constant to a const_int
because of the unalignment:

  /* If VALUE is a floating-point mode, access it as an integer of the
     corresponding size.  This can occur on a machine with 64 bit registers
     that uses SFmode for float.  This can also occur for unaligned float
     structure fields.  */

All this eventually creates a REG_EQUAL note with a const_int, instead
of a const_double:

(insn 11 10 0 (set (reg:SF 120)
        (mem/u/i:SF (lo_sum:SI (reg:SI 121)
                (symbol_ref/u:SI ("*.LC0") [flags 0x2])) [0 S4 A32])) -1 (nil)
    (expr_list:REG_EQUAL (const_int 1202908642 [0x47b2ede2])
        (nil)))

The constant pool will contain a value of (const_int 1202908642) with
a mode of SF.  This will cause output_constant_pool_2() to abort because
we have a MODE_FLOAT, but an integer as a constant.

Is the REG_EQUAL note correct being a const_int?  If so, is the constant
pool correct having SFmode, but a const_int?  If so, then we need
to change output_constant_pool_2() to allow const_int's for MODE_FLOATS.

However...I'm not sure if this is all sane (rtl, and all).

Thoughts?

Aldy

Re: rfc: constant pool and floats

[Andrew Pinski]

On Oct 15, 2004, at 5:30 PM, Aldy Hernandez wrote:

> Hi folks.
>
> The following testcase is failing on powerpc-linux as well as the 
> embedded
> variants.
>
> FAIL: tmpdir-gcc.dg-struct-layout-1/t001 c_compat_x_tst.o compile

Note Janis filed a bug for this, PR 18004.

It also happens for aligned(1) but I thought the definition of aligned
says it never lowers the alignment and packed does that.  Maybe I 
misread
the documentation.

Thanks,
Andrew Pinski

Re: rfc: constant pool and floats

[Daniel Jacobowitz]

On Fri, Oct 15, 2004 at 05:45:36PM -0400, Andrew Pinski wrote:
> 
> On Oct 15, 2004, at 5:30 PM, Aldy Hernandez wrote:
> 
> >Hi folks.
> >
> >The following testcase is failing on powerpc-linux as well as the 
> >embedded
> >variants.
> >
> >FAIL: tmpdir-gcc.dg-struct-layout-1/t001 c_compat_x_tst.o compile
> 
> Note Janis filed a bug for this, PR 18004.
> 
> It also happens for aligned(1) but I thought the definition of aligned
> says it never lowers the alignment and packed does that.  Maybe I 
> misread
> the documentation.

This has been broken for a long time.  See:
 Date: Mon, 12 Jul 2004 19:56:26 -0700                                                                   
 From: Jim Wilson <wilson@specifixinc.com>                                                               
 Subject: Re: Documentation for __aligned__ out of date?                                                 

 That is a bug in the implementation of the aligned attribute.
 Apparently one that has been around for a long time unnoticed.  I tried
 2.8.1 and it fails there also.

 If you try this with a structure type, you will see that it works as
 documented.


-- 
Daniel Jacobowitz

Re: rfc: constant pool and floats

[Aldy Hernandez]

> > Note Janis filed a bug for this, PR 18004.
> > 
> > It also happens for aligned(1) but I thought the definition of aligned
> > says it never lowers the alignment and packed does that.  Maybe I 
> > misread
> > the documentation.

I believe the ICE in this case, is unrelated to the broken __aligned__
behavior.

Aldy

Re: rfc: constant pool and floats

[Aldy Hernandez]

> Note Janis filed a bug for this, PR 18004.
> 
> It also happens for aligned(1) but I thought the definition of aligned
> says it never lowers the alignment and packed does that.  Maybe I 

Yes, the original test had aligned(1).  I was playing with aligned(2)
to see if it made a difference.

Re: rfc: constant pool and floats

[Geoffrey Keating]

Aldy Hernandez <aldyh@redhat.com> writes:

> store_bit_field() changes the floating constant to a const_int
> because of the unalignment:
> 
>   /* If VALUE is a floating-point mode, access it as an integer of the
>      corresponding size.  This can occur on a machine with 64 bit registers
>      that uses SFmode for float.  This can also occur for unaligned float
>      structure fields.  */
> 
> All this eventually creates a REG_EQUAL note with a const_int, instead
> of a const_double:

That's the problem.  Clearly, (reg:SF 120) is not equal to the
CONST_INT; it's equal to a SFmode CONST_DOUBLE.

> (insn 11 10 0 (set (reg:SF 120)
>         (mem/u/i:SF (lo_sum:SI (reg:SI 121)
>                 (symbol_ref/u:SI ("*.LC0") [flags 0x2])) [0 S4 A32])) -1 (nil)
>     (expr_list:REG_EQUAL (const_int 1202908642 [0x47b2ede2])
>         (nil)))

All SFmode constants are CONST_DOUBLEs.  If you have a CONST_INT, you
can use simplify_subreg to turn it into a CONST_DOUBLE.

Re: rfc: constant pool and floats

[Aldy Hernandez]

> That's the problem.  Clearly, (reg:SF 120) is not equal to the
> CONST_INT; it's equal to a SFmode CONST_DOUBLE.

I played around kludging the REG_EQUAL to be equal to a CONST_DOUBLE.
That didn't fix the problem because when we fill in the constant pool
(force_const_mem) with the value, we fill it in with a const_int/SFmode
pair, which still causes the abort while dumping the constant pool.

I can see 3 possible solutions:

(a) Change the const_int appropriately when filling in the constant pool
    (in force_const_mem).

(b) Change the const_int in the backend before we call force_const_mem.

(c) Remove the code creating the const_int in the first place, thus
    keeping the const_double as is.  The offending code is:

  /* If VALUE is a floating-point mode, access it as an integer of the
     corresponding size.  This can occur on a machine with 64 bit registers
     that uses SFmode for float.  This can also occur for unaligned float
     structure fields.  */
  if (GET_MODE_CLASS (GET_MODE (value)) != MODE_INT
      && GET_MODE_CLASS (GET_MODE (value)) != MODE_PARTIAL_INT)
    value = gen_lowpart ((GET_MODE (value) == VOIDmode
			  ? word_mode : int_mode_for_mode (GET_MODE (value))),
			 value);

All 3 options fix the regression, and PR1004.

What do you suggest?  I don't understand if the code in question (snippet 
above) is correct or not.

Thanks in advance.
Aldy

Re: rfc: constant pool and floats

[Geoff Keating]

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]


On 15/10/2004, at 7:30 PM, Aldy Hernandez wrote:

>> That's the problem.  Clearly, (reg:SF 120) is not equal to the
>> CONST_INT; it's equal to a SFmode CONST_DOUBLE.
>
> I played around kludging the REG_EQUAL to be equal to a CONST_DOUBLE.
> That didn't fix the problem because when we fill in the constant pool
> (force_const_mem) with the value, we fill it in with a const_int/SFmode
> pair, which still causes the abort while dumping the constant pool.

Where did the CONST_INT come from, if not the REG_EQUAL note?

>   /* If VALUE is a floating-point mode, access it as an integer of the
>      corresponding size.  This can occur on a machine with 64 bit 
> registers
>      that uses SFmode for float.  This can also occur for unaligned 
> float
>      structure fields.  */
>   if (GET_MODE_CLASS (GET_MODE (value)) != MODE_INT
>       && GET_MODE_CLASS (GET_MODE (value)) != MODE_PARTIAL_INT)
>     value = gen_lowpart ((GET_MODE (value) == VOIDmode
> 			  ? word_mode : int_mode_for_mode (GET_MODE (value))),
> 			 value);

This code is not wrong, as such, but I'm a bit confused about what then 
happens to 'value'.  The routine appears to return it, but there's no 
documentation about what the routine is *supposed* to return, and every 
use of it in the compiler appears to ignore the return value.

Anyway, what you want to look for is the first point where SFmode and 
CONST_INT get associated with each other.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2410 bytes --]

Re: rfc: constant pool and floats

[Aldy Hernandez]

Thanks for taking the time on this.

> >>That's the problem.  Clearly, (reg:SF 120) is not equal to the
> >>CONST_INT; it's equal to a SFmode CONST_DOUBLE.
> >
> >I played around kludging the REG_EQUAL to be equal to a CONST_DOUBLE.
> >That didn't fix the problem because when we fill in the constant pool
> >(force_const_mem) with the value, we fill it in with a const_int/SFmode
> >pair, which still causes the abort while dumping the constant pool.
> 
> Where did the CONST_INT come from, if not the REG_EQUAL note?

Ah, see below.

> >  /* If VALUE is a floating-point mode, access it as an integer of the
> >     corresponding size.  This can occur on a machine with 64 bit 
> >registers
> >     that uses SFmode for float.  This can also occur for unaligned 
> >float
> >     structure fields.  */
> >  if (GET_MODE_CLASS (GET_MODE (value)) != MODE_INT
> >      && GET_MODE_CLASS (GET_MODE (value)) != MODE_PARTIAL_INT)
> >    value = gen_lowpart ((GET_MODE (value) == VOIDmode
> >			  ? word_mode : int_mode_for_mode (GET_MODE 
> >			  (value))),
> >			 value);
> 
> This code is not wrong, as such, but I'm a bit confused about what then 
> happens to 'value'.  The routine appears to return it, but there's no 
> documentation about what the routine is *supposed* to return, and every 
> use of it in the compiler appears to ignore the return value.
> 
> Anyway, what you want to look for is the first point where SFmode and 
> CONST_INT get associated with each other.

The code above sets 'value' to the const_int.  Further down we recurse into
store_bit_field:

          /* Fetch that unit, store the bitfield in it, then store
             the unit.  */
          tempreg = copy_to_reg (op0);
=>        store_bit_field (tempreg, bitsize, bitpos, fieldmode, value);

Where:
	tempreg = (reg:SI 119)
	fieldmode = SFmode
	value = (const_int 1202908642)

Store_bit_field eventually calls emit_move_insn with:
	dst = (subreg:SF (reg:SI 119) 0)
	src = (const_int xx)

*This* is the first time we associate an SF with a const_int.

I'm a bit confused here, so I'm not sure if this is related, but 
emit_move_insn calls rs6000_emit_move with the above 'dst' and 'src',
which calls force_reg(SFmode, (const_int xx)), which is where we get
the REG_EQUAL note:

(insn 11 10 0 (set (reg:SF 120)
        (mem/u/i:SF (lo_sum:SI (reg:SI 121)
                (symbol_ref/u:SI ("*.LC0") [flags 0x2])) [0 S4 A32])) -1 (nil)
    (expr_list:REG_EQUAL (const_int 1202908642 [0x47b2ede2])
        (nil)))

It looks like store_bit_field is at fault by calling the emit_move_insn
with an incompatible source/dest.  Is (set (reg:SF) (const_int)) valid
rtl?  David had mentioned it was, but now I'm not sure.

Aldy

Re: rfc: constant pool and floats

[David Edelsohn]

>>>>> Aldy Hernandez writes:

Aldy> It looks like store_bit_field is at fault by calling the emit_move_insn
Aldy> with an incompatible source/dest.  Is (set (reg:SF) (const_int)) valid
Aldy> rtl?  David had mentioned it was, but now I'm not sure.

	The rs6000 movsf pattern allows that combination, if reg:SF is a
GPR.

David

Re: rfc: constant pool and floats

[Geoff Keating]

[-- Attachment #1: Type: text/plain, Size: 3410 bytes --]


On 16/10/2004, at 6:30 PM, Aldy Hernandez wrote:

> Thanks for taking the time on this.
>
>>>> That's the problem.  Clearly, (reg:SF 120) is not equal to the
>>>> CONST_INT; it's equal to a SFmode CONST_DOUBLE.
>>>
>>> I played around kludging the REG_EQUAL to be equal to a CONST_DOUBLE.
>>> That didn't fix the problem because when we fill in the constant pool
>>> (force_const_mem) with the value, we fill it in with a 
>>> const_int/SFmode
>>> pair, which still causes the abort while dumping the constant pool.
>>
>> Where did the CONST_INT come from, if not the REG_EQUAL note?
>
> Ah, see below.
>
>>>  /* If VALUE is a floating-point mode, access it as an integer of the
>>>     corresponding size.  This can occur on a machine with 64 bit
>>> registers
>>>     that uses SFmode for float.  This can also occur for unaligned
>>> float
>>>     structure fields.  */
>>>  if (GET_MODE_CLASS (GET_MODE (value)) != MODE_INT
>>>      && GET_MODE_CLASS (GET_MODE (value)) != MODE_PARTIAL_INT)
>>>    value = gen_lowpart ((GET_MODE (value) == VOIDmode
>>> 			  ? word_mode : int_mode_for_mode (GET_MODE
>>> 			  (value))),
>>> 			 value);
>>
>> This code is not wrong, as such, but I'm a bit confused about what 
>> then
>> happens to 'value'.  The routine appears to return it, but there's no
>> documentation about what the routine is *supposed* to return, and 
>> every
>> use of it in the compiler appears to ignore the return value.
>>
>> Anyway, what you want to look for is the first point where SFmode and
>> CONST_INT get associated with each other.
>
> The code above sets 'value' to the const_int.  Further down we recurse 
> into
> store_bit_field:
>
>           /* Fetch that unit, store the bitfield in it, then store
>              the unit.  */
>           tempreg = copy_to_reg (op0);
> =>        store_bit_field (tempreg, bitsize, bitpos, fieldmode, value);
>
> Where:
> 	tempreg = (reg:SI 119)
> 	fieldmode = SFmode
> 	value = (const_int 1202908642)
>
> Store_bit_field eventually calls emit_move_insn with:
> 	dst = (subreg:SF (reg:SI 119) 0)
> 	src = (const_int xx)
>
> *This* is the first time we associate an SF with a const_int.

Aha!  So it should not do that.  I think the right thing to do is to 
pass the original VALUE here, but if that doesn't work, try passing the 
mode of VALUE instead of FIELDMODE (the mode of VALUE is the mode that 
got passed to gen_lowpart).

> I'm a bit confused here, so I'm not sure if this is related, but
> emit_move_insn calls rs6000_emit_move with the above 'dst' and 'src',
> which calls force_reg(SFmode, (const_int xx)), which is where we get
> the REG_EQUAL note:
>
> (insn 11 10 0 (set (reg:SF 120)
>         (mem/u/i:SF (lo_sum:SI (reg:SI 121)
>                 (symbol_ref/u:SI ("*.LC0") [flags 0x2])) [0 S4 A32])) 
> -1 (nil)
>     (expr_list:REG_EQUAL (const_int 1202908642 [0x47b2ede2])
>         (nil)))
>
> It looks like store_bit_field is at fault by calling the emit_move_insn
> with an incompatible source/dest.  Is (set (reg:SF) (const_int)) valid
> rtl?  David had mentioned it was, but now I'm not sure.

No, I don't think it's valid RTL; certainly, there is no reason for it 
to be.

David commented:

> The rs6000 movsf pattern allows that combination, if reg:SF is a GPR.

which it does, by using 'n', but I suspect that's just a workaround for 
some old bug (perhaps even this one).  It's been that way since at 
least 1997.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2410 bytes --]

Re: rfc: constant pool and floats

[Aldy Hernandez]

> Aha!  So it should not do that.  I think the right thing to do is to 
> pass the original VALUE here, but if that doesn't work, try passing the 
> mode of VALUE instead of FIELDMODE (the mode of VALUE is the mode that 
> got passed to gen_lowpart).

Neato.  Passing the original value does the trick.

Bootstraped on powerpc-linux.

OK, pending on tests completion?

Aldy

	* expmed.c (store_bit_field): Pass original 'value' before
	recursing.

Index: expmed.c
===================================================================
RCS file: /cvs/uberbaum/gcc/expmed.c,v
retrieving revision 1.198
diff -c -p -r1.198 expmed.c
*** expmed.c	7 Oct 2004 05:56:51 -0000	1.198
--- expmed.c	18 Oct 2004 19:42:03 -0000
*************** store_bit_field (rtx str_rtx, unsigned H
*** 338,343 ****
--- 338,344 ----
    unsigned HOST_WIDE_INT bitpos = bitnum % unit;
    rtx op0 = str_rtx;
    int byte_offset;
+   rtx orig_value;
  
    enum machine_mode op_mode = mode_for_extraction (EP_insv, 3);
  
*************** store_bit_field (rtx str_rtx, unsigned H
*** 601,606 ****
--- 602,608 ----
       corresponding size.  This can occur on a machine with 64 bit registers
       that uses SFmode for float.  This can also occur for unaligned float
       structure fields.  */
+   orig_value = value;
    if (GET_MODE_CLASS (GET_MODE (value)) != MODE_INT
        && GET_MODE_CLASS (GET_MODE (value)) != MODE_PARTIAL_INT)
      value = gen_lowpart ((GET_MODE (value) == VOIDmode
*************** store_bit_field (rtx str_rtx, unsigned H
*** 667,673 ****
  	  /* Fetch that unit, store the bitfield in it, then store
  	     the unit.  */
  	  tempreg = copy_to_reg (op0);
! 	  store_bit_field (tempreg, bitsize, bitpos, fieldmode, value);
  	  emit_move_insn (op0, tempreg);
  	  return value;
  	}
--- 669,675 ----
  	  /* Fetch that unit, store the bitfield in it, then store
  	     the unit.  */
  	  tempreg = copy_to_reg (op0);
! 	  store_bit_field (tempreg, bitsize, bitpos, fieldmode, orig_value);
  	  emit_move_insn (op0, tempreg);
  	  return value;
  	}

Re: rfc: constant pool and floats

[Aldy Hernandez]

> 	* expmed.c (store_bit_field): Pass original 'value' before
> 	recursing.

Oh yeah, this fixes PR 18004.

Re: rfc: constant pool and floats

[Aldy Hernandez]

On Mon, Oct 18, 2004 at 03:46:23PM -0400, Aldy Hernandez wrote:
> > 	* expmed.c (store_bit_field): Pass original 'value' before
> > 	recursing.
> 
> Oh yeah, this fixes PR 18004.

ping...and btw, tests passed, and fixed the regression. :)

Aldy