diff mbox

[AARCH64] PR60034

Message ID 5314FA90.6010807@linaro.org
State Superseded
Headers show

Commit Message

Kugan Vivekanandarajah March 3, 2014, 9:56 p.m. UTC 
On 27/02/14 22:32, Marcus Shawcroft wrote:
> On 21 February 2014 04:24, Kugan <kugan.vivekanandarajah@linaro.org> wrote:
> 
>> Compiling inline asm results in ICE (PR60034). Alignment calculation in
>> aarch64_classify_address for (symbol_ref:DI ("*.LANCHOR4") [flags
>> 0x182])) seems wrong here.
> 
> Hi Kugan,
> 
> +      else if (SYMBOL_REF_FLAGS (sym))
> + align = GET_MODE_ALIGNMENT (GET_MODE (sym));
> 
> This is inserted into the LO_SUM handling in the function
> aarch64_classify_address(), the code in question is checking the
> alignment of the object to ensure that a scaled address instruction
> would be valid. The proposed code is testing if any of a bunch of
> unrelated predicate flags have been set on the symbol and using that
> to gate whether GET_MODE_ALIGNMENT would give accurate alignment
> information on the symbol. I'm not convinced that the presence of
> SYMBOL_REF_FLAGS states anything definitive about the relevance of
> GET_MODE_ALIGNMENT.   The test looks like it fails because a section
> anchor has been introduced and we fail to determine anything sensible
> about the alignment of a section anchor.  How about this instead?
> 
>  if (SYMBOL_REF_BLOCK (sym))
>    align = SYMBOL_REF_BLOCK (sym)->alignment;
> 

Thanks Marcus for the explanation.  I have now changed it based on this
and regression tested on qemu-aarch64 for aarch64-none-linux-gnu with no
new regressions.

Is this OK?

>>
>> Fixing this also  caused a regression for pr38151.c, which is due to
>> complex type being allocated with wrong alignment. Attached patch fixes
>> these issues.
> 
> It ~might~ be beneficial to increase data_alignment here as suggest
> for performance reasons, but the existing alignment should not cause
> breakage... this issue suggest to me that the SYMBOL_REF_FLAGS
> approach is at fault.
> 

Removing this hunk. I will post it as a desperate patch after more analysis.

Thanks,
Kugan


gcc/

2014-03-03  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR target/60034
	* aarch64/aarch64.c (aarch64_classify_address): Fix alignment for
	section anchor.



gcc/testsuite/

2014-03-03  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR target/60034
	* gcc.target/aarch64/pr60034.c: New file.

Comments

Kugan Vivekanandarajah March 12, 2014, 2:05 a.m. UTC | #1 
Ping ?


> 
> 
> gcc/
> 
> 2014-03-03  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 
> 	PR target/60034
> 	* aarch64/aarch64.c (aarch64_classify_address): Fix alignment for
> 	section anchor.
> 
> 
> 
> gcc/testsuite/
> 
> 2014-03-03  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 
> 	PR target/60034
> 	* gcc.target/aarch64/pr60034.c: New file.
>
Marcus Shawcroft March 12, 2014, 9:07 a.m. UTC | #2 
Hi Kugan


On 3 March 2014 21:56, Kugan <kugan.vivekanandarajah@linaro.org> wrote:

> gcc/
>
> 2014-03-03  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         PR target/60034
>         * aarch64/aarch64.c (aarch64_classify_address): Fix alignment for
>         section anchor.
>
>
>
> gcc/testsuite/
>
> 2014-03-03  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         PR target/60034
>         * gcc.target/aarch64/pr60034.c: New file.
>

+      else if (SYMBOL_REF_HAS_BLOCK_INFO_P (sym)

This test makes sense.

+       && SYMBOL_REF_ANCHOR_P (sym)

Do we  need this test  or is the patch being conservative?  I would
have thought that it is sufficient to drop this test and just take the
block alignment...

+       && SYMBOL_REF_BLOCK (sym) != NULL)
+ align = SYMBOL_REF_BLOCK (sym)->alignment;

+/* { dg-options "-std=gnu99 -fgnu89-inline -O -Wall -Winline
-Wwrite-strings -fmerge-all-constants -frounding-math -g
-Wstrict-prototypes" } */

Can you drop all the options that are not actually required to
reproduce the issue?

Cheers
/Marcus
Kugan Vivekanandarajah March 12, 2014, 9:24 a.m. UTC | #3 
On 12/03/14 20:07, Marcus Shawcroft wrote:
> Hi Kugan
> 
> 
> On 3 March 2014 21:56, Kugan <kugan.vivekanandarajah@linaro.org> wrote:
> 
>> gcc/
>>
>> 2014-03-03  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>
>>         PR target/60034
>>         * aarch64/aarch64.c (aarch64_classify_address): Fix alignment for
>>         section anchor.
>>
>>
>>
>> gcc/testsuite/
>>
>> 2014-03-03  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>
>>         PR target/60034
>>         * gcc.target/aarch64/pr60034.c: New file.
>>
> 
> +      else if (SYMBOL_REF_HAS_BLOCK_INFO_P (sym)
> 
> This test makes sense.
> 
> +       && SYMBOL_REF_ANCHOR_P (sym)
> 
> Do we  need this test  or is the patch being conservative?  I would
> have thought that it is sufficient to drop this test and just take the
> block alignment...
> 
Thanks for the review.

If I understand gcc/rtl.h correctly, SYMBOL_REF_ANCHOR_P (sym) is
required for anchor SYMBOL_REFS. SYMBOL_REF_BLOCK (sym) != NULL is
probably redundant. This can probably become an gcc_assert
(SYMBOL_REF_BLOCK (sym)) instead.


> +       && SYMBOL_REF_BLOCK (sym) != NULL)
> + align = SYMBOL_REF_BLOCK (sym)->alignment;
> 
> +/* { dg-options "-std=gnu99 -fgnu89-inline -O -Wall -Winline
> -Wwrite-strings -fmerge-all-constants -frounding-math -g
> -Wstrict-prototypes" } */
> 
> Can you drop all the options that are not actually required to
> reproduce the issue?

I will change it.


Thanks,
Kugan
Marcus Shawcroft March 12, 2014, 10:01 a.m. UTC | #4 
>> +      else if (SYMBOL_REF_HAS_BLOCK_INFO_P (sym)
>>
>> This test makes sense.
>>
>> +       && SYMBOL_REF_ANCHOR_P (sym)
>>
>> Do we  need this test  or is the patch being conservative?  I would
>> have thought that it is sufficient to drop this test and just take the
>> block alignment...
>>
> Thanks for the review.
>
> If I understand gcc/rtl.h correctly, SYMBOL_REF_ANCHOR_P (sym) is
> required for anchor SYMBOL_REFS. SYMBOL_REF_BLOCK (sym) != NULL is
> probably redundant. This can probably become an gcc_assert
> (SYMBOL_REF_BLOCK (sym)) instead.

I agree with your interpretation of the code and comments in rtl.h.  I
also accept that SYMBOL_REF_ANCHOR_P() is sufficient to resolve the
test case.  However I'm wondering why we need to constraint the test
down to SYMBOL_REF_ANCHOR_P().  At this point in the code we are
trying to find alignment of the object, if we have a SYMBOL_REF_BLOCK
then we can get the block alignment irrespective of
SYMBOL_REF_ANCHOR_P().

Cheers
/Marcus
diff mbox

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 901ad3d..d2a9217 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3199,6 +3199,10 @@  aarch64_classify_address (struct aarch64_address_info *info,
 		}
 	      else if (SYMBOL_REF_DECL (sym))
 		align = DECL_ALIGN (SYMBOL_REF_DECL (sym));
+	      else if (SYMBOL_REF_HAS_BLOCK_INFO_P (sym)
+		       && SYMBOL_REF_ANCHOR_P (sym)
+		       && SYMBOL_REF_BLOCK (sym) != NULL)
+		align = SYMBOL_REF_BLOCK (sym)->alignment;
 	      else
 		align = BITS_PER_UNIT;
 
diff --git a/gcc/testsuite/gcc.target/aarch64/pr60034.c b/gcc/testsuite/gcc.target/aarch64/pr60034.c
index e69de29..d126779 100644
--- a/gcc/testsuite/gcc.target/aarch64/pr60034.c
+++ b/gcc/testsuite/gcc.target/aarch64/pr60034.c
@@ -0,0 +1,10 @@ 
+/* { dg-do compile } */
+/* { dg-options "-std=gnu99 -fgnu89-inline -O -Wall -Winline -Wwrite-strings -fmerge-all-constants -frounding-math -g -Wstrict-prototypes" } */
+
+static unsigned long global_max_fast;
+
+void __libc_mallopt (int param_number, int value)
+{
+ __asm__ __volatile__ ("# %[_SDT_A21]" :: [_SDT_A21] "nr" ((global_max_fast)));
+ global_max_fast = 1;
+}