Patchwork [RFC] PR 55403 + 55391

login
register
mail settings
Submitter Richard Sandiford
Date Nov. 20, 2012, 5:29 p.m.
Message ID <87pq38uqr3.fsf@sandifor-thinkpad.stglab.manchester.uk.ibm.com>
Download mbox | patch
Permalink /patch/200489/
State New
Headers show

Comments

Richard Sandiford - Nov. 20, 2012, 5:29 p.m.
Richard Henderson <rth@redhat.com> writes:
> On 11/20/2012 08:55 AM, Richard Sandiford wrote:
>> but what kind of bitfield memory were we trying to
>> create in the ICE case?  The idea was that "adjust_object" is only ever
>> true for bitfield adjustments.  We should then either be using an
>> integer or field mode whose size is picked up by:
>> 
>>   if (defattrs->size_known_p)
>>     size = defattrs->size;
>> 
>> or a BLKmode whose value is passed in via adjust_bitfield_address_size.
>> It sounds like I missed a case where the latter was needed.
>
> A TFmode field of an unaligned TCmode original.  We do wind up with
> a BLKmode extraction, without the _size passed in.

Gah.  How about this patch, currently bootstrapping on x86_64-linux-gnu
as a sanity check?  The last instance seems glaringly obvious in
hindsight :-(

Richard


gcc/
	* expmed.c (store_bit_field_1): Use adjust_bitfield_address_size
	rather than adjust_bitfield_address to change the mode of a reference.
	(extract_bit_field_1): Likewise.
Richard Henderson - Nov. 20, 2012, 7:24 p.m.
On 11/20/2012 09:29 AM, Richard Sandiford wrote:
> Gah.  How about this patch, currently bootstrapping on x86_64-linux-gnu
> as a sanity check?  The last instance seems glaringly obvious in
> hindsight :-(
> 
> Richard
> 
> 
> gcc/
> 	* expmed.c (store_bit_field_1): Use adjust_bitfield_address_size
> 	rather than adjust_bitfield_address to change the mode of a reference.
> 	(extract_bit_field_1): Likewise.

That patch does fix my ICE.

It looks all good, too.  As you say, glaringly obvious even.  ;-)


r~
Richard Henderson - Nov. 20, 2012, 8:04 p.m.
On 11/20/2012 11:24 AM, Richard Henderson wrote:
> On 11/20/2012 09:29 AM, Richard Sandiford wrote:
>> Gah.  How about this patch, currently bootstrapping on x86_64-linux-gnu
>> as a sanity check?  The last instance seems glaringly obvious in
>> hindsight :-(
>>
>> Richard
>>
>>
>> gcc/
>> 	* expmed.c (store_bit_field_1): Use adjust_bitfield_address_size
>> 	rather than adjust_bitfield_address to change the mode of a reference.
>> 	(extract_bit_field_1): Likewise.
> 
> That patch does fix my ICE.
> 
> It looks all good, too.  As you say, glaringly obvious even.  ;-)

One further point -- get_best_mem_extraction_insn does not work for
the traditional 'extv' patterns that only accept memories.  In
particular, the QImode memory in the extv pattern never matches up
at the beginning of get_traditional_extraction_insn, so that first
"if (mode != struct_mode) return false;" always triggers.

I audited the existing extv patterns and the affected targets are
alpha, sh, and vax.  All of the others implement extv on registers,
which appears to work.  Test case:

struct S { long y __attribute__((packed)); };
long g(struct S *s) { return s->y; }

Before:

	ldq_u $0,0($16)
	ldq_u $1,7($16)
	extql $0,$16,$0
	extqh $1,$16,$16
	bis $0,$16,$0

After:

	ldbu $5,1($16)
	ldbu $8,0($16)
	ldbu $7,2($16)
	ldbu $6,3($16)
	ldbu $4,4($16)
	ldbu $3,5($16)
	sll $5,8,$5
	ldbu $2,6($16)
	ldbu $1,7($16)
	sll $7,16,$7
	sll $6,24,$6
	bis $5,$8,$5
	sll $4,32,$4
	sll $3,40,$3
	bis $7,$5,$5
	sll $2,48,$2
	sll $1,56,$1
	bis $6,$5,$0
	bis $4,$0,$0
	bis $3,$0,$0
	bis $2,$0,$0
	bis $1,$0,$0

I suppose the question is: with only 3 affected targets, is it more
trouble fiddling the somewhat confused "traditional" path, or to
just go ahead and update the backends?


r~

Patch

Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	2012-11-20 09:55:26.000000000 +0000
+++ gcc/expmed.c	2012-11-20 17:24:09.722871322 +0000
@@ -645,7 +645,7 @@  store_bit_field_1 (rtx str_rtx, unsigned
     if (imode != GET_MODE (op0))
       {
 	if (MEM_P (op0))
-	  op0 = adjust_bitfield_address (op0, imode, 0);
+	  op0 = adjust_bitfield_address_size (op0, imode, 0, MEM_SIZE (op0));
 	else
 	  {
 	    gcc_assert (imode != BLKmode);
@@ -1380,7 +1380,7 @@  extract_bit_field_1 (rtx str_rtx, unsign
     if (imode != GET_MODE (op0))
       {
 	if (MEM_P (op0))
-	  op0 = adjust_bitfield_address (op0, imode, 0);
+	  op0 = adjust_bitfield_address_size (op0, imode, 0, MEM_SIZE (op0));
 	else if (imode != BLKmode)
 	  {
 	    op0 = gen_lowpart (imode, op0);
@@ -1403,10 +1403,10 @@  extract_bit_field_1 (rtx str_rtx, unsign
 	  }
 	else
 	  {
-	    rtx mem = assign_stack_temp (GET_MODE (op0),
-					 GET_MODE_SIZE (GET_MODE (op0)));
+	    HOST_WIDE_INT size = GET_MODE_SIZE (GET_MODE (op0));
+	    rtx mem = assign_stack_temp (GET_MODE (op0), size);
 	    emit_move_insn (mem, op0);
-	    op0 = adjust_bitfield_address (mem, BLKmode, 0);
+	    op0 = adjust_bitfield_address_size (mem, BLKmode, 0, size);
 	  }
       }
   }