On Tue, Nov 11, 2014 at 3:40 PM, Thomas Preud'homme
<thomas.preudho...@arm.com> wrote:
> Fixed the subject.
>
> Best regards,
>
> Thomas
>
>> -----Original Message-----
>> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
>> ow...@gcc.gnu.org] On Behalf Of Thomas Preud'homme
>> Sent: Tuesday, November 11, 2014 3:31 PM
>> To: gcc-patches@gcc.gnu.org; Ramana Radhakrishnan; Richard Earnshaw
>> Subject: [PATCH][ARM] Fix PR59593/PR63742: arm *movhi_insn_arch4
>> pattern for big-endian
>>
>> Currently, constant pool entries for QImode, HImode and SImode values
>> are filled as 32-bit quantities. This works fine for little endian system but
>> gives some incorrect results for big endian system when the value is
>> *accessed* with a mode smaller than 32-bit in size. Suppose the case of
>> the value 0x1234 that is accessed as an HImode value. It would be output
>> as 0x0 0x0 0x12 0x34 in a constant pool entry and the 16-bit load that
>> would be done would lead to the value 0x0 in register.
>>
>> This patch makes the consttable_1 and consttable_2 pattern available to
>> ARM as well so that values are output according to their mode. This is a
>> different approach than the one proposed by Felix Yang at [1].
>>
>> [1] https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00258.html
>>
>> ChangeLog entries are as follows:
>>
>> *** gcc/ChangeLog ***
>>
>> 2014-09-03 Thomas Preud'homme <thomas.preudho...@arm.com>
>>
>> PR target/59593
>> * config/arm/arm.c (dump_minipool): dispatch to consttable
>> pattern
>> based on mode size.
>> * config/arm/arm.md (consttable_1): Move from
>> config/arm/thumb1.md and
>> make it TARGET_EITHER.
>> (consttable_2): Move from config/arm/thumb1.md, make it
>> TARGET_EITHER
>> and move HFmode handling from consttable_4 to it.
>> (consttable_4): Move HFmode handling to consttable_2 pattern.
>> * config/arm/thumb1.md (consttable_1): Move to
>> config/arm/arm.md.
>> (consttable_2): Ditto.
>>
>> *** gcc/testsuite/ChangeLog ***
>>
>> 2014-10-08 Thomas Preud'homme <thomas.preudho...@arm.com>
>>
>> PR target/59593
>> * gcc.target/arm/constant-pool.c: New test.
>>
>>
>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>> index d8bfda3..63babd2 100644
>> --- a/gcc/config/arm/arm.c
>> +++ b/gcc/config/arm/arm.c
>> @@ -16608,7 +16608,7 @@ dump_minipool (rtx_insn *scan)
>> fputc ('\n', dump_file);
>> }
>>
>> - switch (mp->fix_size)
>> + switch (GET_MODE_SIZE (mp->mode))
>> {
>> #ifdef HAVE_consttable_1
>> case 1:
>> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
>> index cd9ab6c..8ca88b6 100644
>> --- a/gcc/config/arm/arm.md
>> +++ b/gcc/config/arm/arm.md
>> @@ -10567,6 +10567,42 @@
>> [(set_attr "type" "no_insn")]
>> )
>>
>> +(define_insn "consttable_1"
>> + [(unspec_volatile [(match_operand 0 "" "")] VUNSPEC_POOL_1)]
>> + "TARGET_EITHER"
>> + "*
>> + making_const_table = TRUE;
>> + assemble_integer (operands[0], 1, BITS_PER_WORD, 1);
>> + assemble_zeros (3);
>> + return \"\";
>> + "
>> + [(set_attr "length" "4")
>> + (set_attr "type" "no_insn")]
>> +)
>> +
>> +(define_insn "consttable_2"
>> + [(unspec_volatile [(match_operand 0 "" "")] VUNSPEC_POOL_2)]
>> + "TARGET_EITHER"
>> + "*
>> + {
>> + rtx x = operands[0];
>> + making_const_table = TRUE;
>> + switch (GET_MODE_CLASS (GET_MODE (x)))
>> + {
>> + case MODE_FLOAT:
>> + arm_emit_fp16_const (x);
>> + break;
>> + default:
>> + assemble_integer (operands[0], 2, BITS_PER_WORD, 1);
>> + assemble_zeros (2);
>> + break;
>> + }
>> + return \"\";
>> + }"
>> + [(set_attr "length" "4")
>> + (set_attr "type" "no_insn")]
>> +)
>> +
>> (define_insn "consttable_4"
>> [(unspec_volatile [(match_operand 0 "" "")] VUNSPEC_POOL_4)]
>> "TARGET_EITHER"
>> @@ -10577,15 +10613,12 @@
>> switch (GET_MODE_CLASS (GET_MODE (x)))
>> {
>> case MODE_FLOAT:
>> - if (GET_MODE (x) == HFmode)
>> - arm_emit_fp16_const (x);
>> - else
>> - {
>> - REAL_VALUE_TYPE r;
>> - REAL_VALUE_FROM_CONST_DOUBLE (r, x);
>> - assemble_real (r, GET_MODE (x), BITS_PER_WORD);
>> - }
>> - break;
>> + {
>> + REAL_VALUE_TYPE r;
>> + REAL_VALUE_FROM_CONST_DOUBLE (r, x);
>> + assemble_real (r, GET_MODE (x), BITS_PER_WORD);
>> + break;
>> + }
>> default:
>> /* XXX: Sometimes gcc does something really dumb and ends up
>> with
>> a HIGH in a constant pool entry, usually because it's trying to
>> diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md
>> index 020d83b..80d470b 100644
>> --- a/gcc/config/arm/thumb1.md
>> +++ b/gcc/config/arm/thumb1.md
>> @@ -1735,33 +1735,6 @@
>> (set_attr "conds" "clob")]
>> )
>>
>> -(define_insn "consttable_1"
>> - [(unspec_volatile [(match_operand 0 "" "")] VUNSPEC_POOL_1)]
>> - "TARGET_THUMB1"
>> - "*
>> - making_const_table = TRUE;
>> - assemble_integer (operands[0], 1, BITS_PER_WORD, 1);
>> - assemble_zeros (3);
>> - return \"\";
>> - "
>> - [(set_attr "length" "4")
>> - (set_attr "type" "no_insn")]
>> -)
>> -
>> -(define_insn "consttable_2"
>> - [(unspec_volatile [(match_operand 0 "" "")] VUNSPEC_POOL_2)]
>> - "TARGET_THUMB1"
>> - "*
>> - making_const_table = TRUE;
>> - gcc_assert (GET_MODE_CLASS (GET_MODE (operands[0])) !=
>> MODE_FLOAT);
>> - assemble_integer (operands[0], 2, BITS_PER_WORD, 1);
>> - assemble_zeros (2);
>> - return \"\";
>> - "
>> - [(set_attr "length" "4")
>> - (set_attr "type" "no_insn")]
>> -)
>> -
>> ;; Miscellaneous Thumb patterns
>> (define_expand "tablejump"
>> [(parallel [(set (pc) (match_operand:SI 0 "register_operand" ""))
>> diff --git a/gcc/testsuite/gcc.target/arm/constant-pool.c
>> b/gcc/testsuite/gcc.target/arm/constant-pool.c
>> new file mode 100644
>> index 0000000..e8209f6
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/constant-pool.c
>> @@ -0,0 +1,27 @@
>> +/* { dg-do run { target armeb-*-*eabi* } } */
Remove the "target" specifier here. Let it run on both big endian and
little endian.
This is OK - thanks.
Ramana
>> +/* { dg-options "-O1" } */
>> +
>> +unsigned short v = 0x5678;
>> +int i;
>> +int j = 0;
>> +int *ptr = &j;
>> +
>> +int
>> +func (void)
>> +{
>> + for (i = 0; i < 1; ++i)
>> + {
>> + *ptr = -1;
>> + v = 0x1234;
>> + }
>> + return v;
>> +}
>> +
>> +int
>> +main (void)
>> +{
>> + func ();
>> + if (v != 0x1234)
>> + __builtin_abort ();
>> + return 0;
>> +}
>>
>>
>> Is it ok for trunk?
>>
>> Best regards,
>>
>> Thomas
>>
>>
>>
>
>
>
>
