diff mbox

[RFC] PR 55403 + 55391

Message ID 87r4nouhai.fsf@talisman.default
State New
Headers show

Commit Message

Richard Sandiford Nov. 20, 2012, 8:54 p.m. UTC 
Richard Henderson <rth@redhat.com> writes:
> 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.

Sorry for all the mishaps.

> 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?

I suppose updating them would be the ideal eventually, but I'd still
like to fix the bug.

I belatedly did a similar audit and it looks there are no patterns that
use a mixture of field and structure modes for register operations.
They're either equal or not present.  So how about the patch below?
I checked that it fixes this testcase.

Richard


gcc/
	* optabs.c (get_traditional_extraction_insn): Check the field mode
	against the given mode.  Only check the structure mode for register
	insertions and extractions.

Comments

Richard Henderson Nov. 20, 2012, 9:18 p.m. UTC | #1 
On 11/20/2012 12:54 PM, Richard Sandiford wrote:
> gcc/
> 	* optabs.c (get_traditional_extraction_insn): Check the field mode
> 	against the given mode.  Only check the structure mode for register
> 	insertions and extractions.

OK.

Thanks for the quick attention.


r~
diff mbox

Patch

Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c	2012-11-18 17:33:42.000000000 +0000
+++ gcc/optabs.c	2012-11-20 20:46:16.893286686 +0000
@@ -8258,11 +8258,12 @@  #define CODE_FOR_extzv	CODE_FOR_nothing
 enum extraction_type { ET_unaligned_mem, ET_reg };
 
 /* Check whether insv, extv or extzv pattern ICODE can be used for an
-   insertion or extraction of type TYPE on a structure of mode MODE.
-   Return true if so and fill in *INSN accordingly.  STRUCT_OP is the
-   operand number of the structure (the first sign_extract or zero_extract
-   operand) and FIELD_OP is the operand number of the field (the other
-   side of the set from the sign_extract or zero_extract).  */
+   insertion or extraction of type TYPE on a structure and field of
+   mode MODE.  Return true if so and fill in *INSN accordingly.
+   STRUCT_OP is the operand number of the structure (the first
+   sign_extract or zero_extract operand) and FIELD_OP is the operand
+   number of the field (the other side of the set from the sign_extract
+   or zero_extract).  */
 
 static bool
 get_traditional_extraction_insn (extraction_insn *insn,
@@ -8273,15 +8274,23 @@  get_traditional_extraction_insn (extract
 {
   const struct insn_data_d *data = &insn_data[icode];
 
-  enum machine_mode struct_mode = data->operand[struct_op].mode;
-  if (struct_mode == VOIDmode)
-    struct_mode = word_mode;
-  if (mode != struct_mode)
-    return false;
-
   enum machine_mode field_mode = data->operand[field_op].mode;
   if (field_mode == VOIDmode)
     field_mode = word_mode;
+  if (mode != field_mode)
+    return false;
+
+  enum machine_mode struct_mode;
+  if (type == ET_unaligned_mem)
+    struct_mode = byte_mode;
+  else
+    {
+      struct_mode = data->operand[struct_op].mode;
+      if (struct_mode == VOIDmode)
+	struct_mode = word_mode;
+      if (mode != struct_mode)
+	return false;
+    }
 
   enum machine_mode pos_mode = data->operand[struct_op + 2].mode;
   if (pos_mode == VOIDmode)
@@ -8289,7 +8298,7 @@  get_traditional_extraction_insn (extract
 
   insn->icode = icode;
   insn->field_mode = field_mode;
-  insn->struct_mode = (type == ET_unaligned_mem ? byte_mode : struct_mode);
+  insn->struct_mode = struct_mode;
   insn->pos_mode = pos_mode;
   return true;
 }