Re: [PATCH][AARCH64]PR60034

[Marcus Shawcroft]

On 25 March 2014 04:55, Kugan kugan.vivekanandara...@linaro.org wrote:

 gcc/

 2014-03-25  Kugan Vivekanandarajah  kug...@linaro.org

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

 gcc/testsuite/

 2014-03-25  Kugan Vivekanandarajah  kug...@linaro.org

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

Thanks Kugan, leave 24h before committing to give the RM's chance to object.
/Marcus

Re: [PATCH][AARCH64]PR60034

[Kugan]

 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().

Thanks for the explanation.  Is the attached patch looks OK ?

Thanks,
Kugan



gcc/

2014-03-25  Kugan Vivekanandarajah  kug...@linaro.org

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



gcc/testsuite/

2014-03-25  Kugan Vivekanandarajah  kug...@linaro.org

PR target/60034
* gcc.target/aarch64/pr60034.c: New file.
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 57b6645..7d2d10c 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3193,6 +3193,9 @@ 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_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..ab7e7f4 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 -O } */
+
+static unsigned long global_max_fast;
+
+void __libc_mallopt (int param_number, int value)
+{
+ __asm__ __volatile__ (# %[_SDT_A21] :: [_SDT_A21] nor 
((global_max_fast)));
+ global_max_fast = 1;
+}

Re: [PATCH][AARCH64]PR60034

[Marcus Shawcroft]

Hi Kugan


On 3 March 2014 21:56, Kugan kugan.vivekanandara...@linaro.org wrote:

 gcc/

 2014-03-03  Kugan Vivekanandarajah  kug...@linaro.org

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



 gcc/testsuite/

 2014-03-03  Kugan Vivekanandarajah  kug...@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

Re: [PATCH][AARCH64]PR60034

[Kugan]

On 12/03/14 20:07, Marcus Shawcroft wrote:
 Hi Kugan
 
 
 On 3 March 2014 21:56, Kugan kugan.vivekanandara...@linaro.org wrote:
 
 gcc/

 2014-03-03  Kugan Vivekanandarajah  kug...@linaro.org

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



 gcc/testsuite/

 2014-03-03  Kugan Vivekanandarajah  kug...@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

Re: [PATCH][AARCH64]PR60034

[Marcus Shawcroft]

 +  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

Re: [PATCH][AARCH64]PR60034

[Kugan]

Ping ?


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

Re: [PATCH][AARCH64]PR60034

[Kugan]

On 27/02/14 22:32, Marcus Shawcroft wrote:
 On 21 February 2014 04:24, Kugan kugan.vivekanandara...@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  kug...@linaro.org

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



gcc/testsuite/

2014-03-03  Kugan Vivekanandarajah  kug...@linaro.org

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

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;
+}

Re: [PATCH][AARCH64]PR60034

[Marcus Shawcroft]

On 21 February 2014 04:24, Kugan kugan.vivekanandara...@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;


 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.

Cheers
/Marcus

[PATCH][AARCH64]PR60034

[Kugan]

Hi all,

Compiling inline asm results in ICE (PR60034). Alignment calculation in
aarch64_classify_address for (symbol_ref:DI (*.LANCHOR4) [flags
0x182])) seems wrong here.

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.

Regression tested on qemu-aarch64 for aarch64-none-linux-gnu with no new
regressions.

Is this patch OK?

Thanks,
Kugan

gcc/
+2014-02-21  Kugan Vivekanandarajah  kug...@linaro.org
+
+   PR target/60034
+   * aarch64/aarch64.c (aarch64_classify_address): Fix alignment for
+   SYMBOL_REF_FLAGS.
+   * aarch64/aarch64.h (DATA_ALIGNMENT):  Fix alignment for COMPLEX_TYPE.
+

gcc/testsuite/
+2014-02-21  Kugan Vivekanandarajah  kug...@linaro.org
+
+   PR target/60034
+   * gcc.target/aarch64/pr60034.c: New file.
+
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index ea90311..89187c0 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3203,6 +3203,8 @@ 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_FLAGS (sym))
+   align = GET_MODE_ALIGNMENT (GET_MODE (sym));
  else
align = BITS_PER_UNIT;
 
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 13c424c..b93e9da 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -132,6 +132,7 @@
   ALIGN)  BITS_PER_WORD)  \
  (TREE_CODE (EXP) == ARRAY_TYPE  \
|| TREE_CODE (EXP) == UNION_TYPE\
+   || TREE_CODE (EXP) == COMPLEX_TYPE  \
|| TREE_CODE (EXP) == RECORD_TYPE)) \
? BITS_PER_WORD : (ALIGN))
 
diff --git a/gcc/testsuite/gcc.target/aarch64/pr60034.c 
b/gcc/testsuite/gcc.target/aarch64/pr60034.c
index e69de29..99c821d 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;
+
+int __libc_mallopt (int param_number, int value)
+{
+ __asm__ __volatile__ (# %[_SDT_A21] :: [_SDT_A21] nr ((global_max_fast)));
+ global_max_fast = 1;
+}