Re: [PATCH, ARM] Fix incorrect alignment of small values in minipool

[Richard Earnshaw <rearnsha at arm dot com>]

On 11/08/14 09:53, Richard Earnshaw wrote:
> On 11/08/14 08:49, Thomas Preud'homme wrote:
>> Being 32-bit wide in size, constant pool entries are always 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.
>> The approach here is to transform the value so that it is output correctly by
>> shifting the value left so that the highest byte in its mode ends up in the
>> highest byte of the 32-bit value being output.
>>
>> The patch was tested by running the testsuite with three different builds of GCC:
>> 1) bootstrap of GCC on x86_64-linux-gnu
>> 2) arm-none-eabi cross compiler (defaulting to little endian) with testsuite
>> run under qemu emulqting a Cortex M4
>> 3) arm-none-eabi cross compiler (defaulting to big endian, thanks to patch at [1])
>> with testsuite run under qemu emulating a Cortex M3. Due to the processor used,
>> the test constant-minipool was not run as part of the testsuite but manually with
>> -cpu=cortex-r4
>>
>> [1] https://sourceware.org/ml/binutils/2014-08/msg00014.html
>>
>> The ChangeLog is as follows:
>>
>> *** gcc/ChangeLog ***
>>
>> 2014-07-14  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>>
>> 	* config/arm/arm.c (dump_minipool): Fix alignment in minipool of
>> 	values whose size is less than MINIPOOL_FIX_SIZE on big endian target.
>>
> 
> I think this is the wrong place for this sort of fix up.  HFmode values
> are fixed up in the consttable_4 pattern and it looks wrong to be that
> HImode values are then fixed up in a different place.  We should be
> consistent and do all the fix ups in one location.
> 

BTW, this is PR59593, please can you cite that in your change log entry.

R.

> R.
> 
> 
>> *** gcc/testsuite/ChangeLog ***
>>
>> 2014-07-14  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>>
>> 	* gcc.target/arm/constant-pool.c: New test.
>>
>>
>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>> index 0146fe8..e4e0ef4 100644
>> --- a/gcc/config/arm/arm.c
>> +++ b/gcc/config/arm/arm.c
>> @@ -16507,6 +16507,15 @@ dump_minipool (rtx scan)
>>  	      fputc ('\n', dump_file);
>>  	    }
>>  
>> +	  /* On big-endian target, make sure that padding for values whose
>> +	     mode size is smaller than MINIPOOL_FIX_SIZE comes after.  */
>> +	  if (BYTES_BIG_ENDIAN && CONST_INT_P (mp->value))
>> +	    {
>> +	      int byte_disp = mp->fix_size - GET_MODE_SIZE (mp->mode);
>> +	      HOST_WIDE_INT val = INTVAL (mp->value);
>> +	      mp->value = GEN_INT (val << (byte_disp * BITS_PER_UNIT));
>> +	    }
>> +
>>  	  switch (mp->fix_size)
>>  	    {
>>  #ifdef HAVE_consttable_1
>> 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..46a1534
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/constant-pool.c
>> @@ -0,0 +1,28 @@
>> +/* { dg-do compile } */
>> +/* { dg-require-effective-target arm_arm_ok } */
>> +/* { dg-options "-O1 -marm -mbig-endian" } */
>> +
>> +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 ();
>> +  return v - 0x1234;
>> +}
>> +
>> +/* { dg-final { scan-assembler-not ".word 4660" } } */
>>
>>
>> Is this ok for trunk?
>>
>> Best regards,
>>
>> Thomas
>>
>>
>>
> 
> 
>