Richard Henderson <r...@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.
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;
}
