[PATCH] varasm: Propagate litpool decl alignment to generated RTX.

[PATCH] varasm: Propagate litpool decl alignment to generated RTX.

[Andreas Krebbel]

When pushing a value into the literal pool the resulting decl might
get a higher alignment than the original expression depending on how a
target defines CONSTANT_ALIGNMENT.  Generating an RTX for the constant
pool access we currently use the alignment from the original
expression.  Changed with the attached patch.

This fixes a GCC 6 regression for S/390.  For arrays of string
constants as in the attached testcase encode_section_info is not able
to figure out that the constant pool slot is already properly aligned
since the mem_align field in the rtx is not set properly.

Another question is why (at the end of build_constant_desc) we invoke
the encode_section_info hook with the original expression instead of
the newly generated var_decl?  I think the hook is supposed to be
invoked with a decl?!

  /* Set flags or add text to the name to record information, such as
     that it is a local symbol.  If the name is changed, the macro
     ASM_OUTPUT_LABELREF will have to know how to strip this
     information.  This call might invalidate our local variable
     SYMBOL; we can't use it afterward.  */
  targetm.encode_section_info (exp, rtl, true);

  desc->rtl = rtl;

  return desc;
}

Bootstrapped and regtested on x86_64 and s390x.

No regressions.

Ok?

-Andreas-

gcc/ChangeLog:

2016-12-16  Andreas Krebbel  <krebbel@linux.vnet.ibm.com>

	* varasm.c (build_constant_desc): Use the alignment of the var
	decl instead of the original expression.

gcc/testsuite/ChangeLog:

2016-12-16  Andreas Krebbel  <krebbel@linux.vnet.ibm.com>

	* gcc.target/s390/litpool-str-1.c: New test.
---
 gcc/testsuite/gcc.target/s390/litpool-str-1.c | 22 ++++++++++++++++++++++
 gcc/varasm.c                                  |  4 ++++
 2 files changed, 26 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/s390/litpool-str-1.c

diff --git a/gcc/testsuite/gcc.target/s390/litpool-str-1.c b/gcc/testsuite/gcc.target/s390/litpool-str-1.c
new file mode 100644
index 0000000..cd921d2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/s390/litpool-str-1.c
@@ -0,0 +1,22 @@
+/* Make sure strings are recognized as being accessible through larl.
+   This requires the symbol ref alignment properly propagated to
+   encode_section_info.  */
+
+/* { dg-do compile } */
+/* { dg-options "-march=z900 -O2 -fpic" } */
+
+
+extern void foo(const char*, const char*, const char*);
+
+void bar(int i)
+{
+  const char t1[10] = "test";
+  const char t2[10] = "test2";
+  const char t3[2][10] = {
+       "foofoofoo",
+       "barbarbar",
+    };
+  foo(t1, t2, t3[i]);
+}
+
+/* { dg-final { scan-assembler-not "GOTOFF" } } */
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 5b15847..68a7ba2 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -3296,6 +3296,10 @@ build_constant_desc (tree exp)
   set_mem_attributes (rtl, exp, 1);
   set_mem_alias_set (rtl, 0);
 
+  /* Putting EXP into the literal pool might have imposed a larger
+     alignment which should be visible in the RTX as well.  */
+  set_mem_align (rtl, DECL_ALIGN (decl));
+
   /* We cannot share RTX'es in pool entries.
      Mark this piece of RTL as required for unsharing.  */
   RTX_FLAG (rtl, used) = 1;
-- 
2.9.1

Re: [PATCH] varasm: Propagate litpool decl alignment to generated RTX.

[Richard Biener]

On Fri, Dec 16, 2016 at 9:29 PM, Andreas Krebbel
<krebbel@linux.vnet.ibm.com> wrote:
> When pushing a value into the literal pool the resulting decl might
> get a higher alignment than the original expression depending on how a
> target defines CONSTANT_ALIGNMENT.  Generating an RTX for the constant
> pool access we currently use the alignment from the original
> expression.  Changed with the attached patch.

And it might be even smaller alignment...  or do we not allow that?

> This fixes a GCC 6 regression for S/390.  For arrays of string
> constants as in the attached testcase encode_section_info is not able
> to figure out that the constant pool slot is already properly aligned
> since the mem_align field in the rtx is not set properly.
>
> Another question is why (at the end of build_constant_desc) we invoke
> the encode_section_info hook with the original expression instead of
> the newly generated var_decl?  I think the hook is supposed to be
> invoked with a decl?!

No idea...

>   /* Set flags or add text to the name to record information, such as
>      that it is a local symbol.  If the name is changed, the macro
>      ASM_OUTPUT_LABELREF will have to know how to strip this
>      information.  This call might invalidate our local variable
>      SYMBOL; we can't use it afterward.  */
>   targetm.encode_section_info (exp, rtl, true);
>
>   desc->rtl = rtl;
>
>   return desc;
> }
>
> Bootstrapped and regtested on x86_64 and s390x.
>
> No regressions.
>
> Ok?

Ok.

Richard.

> -Andreas-
>
> gcc/ChangeLog:
>
> 2016-12-16  Andreas Krebbel  <krebbel@linux.vnet.ibm.com>
>
>         * varasm.c (build_constant_desc): Use the alignment of the var
>         decl instead of the original expression.
>
> gcc/testsuite/ChangeLog:
>
> 2016-12-16  Andreas Krebbel  <krebbel@linux.vnet.ibm.com>
>
>         * gcc.target/s390/litpool-str-1.c: New test.
> ---
>  gcc/testsuite/gcc.target/s390/litpool-str-1.c | 22 ++++++++++++++++++++++
>  gcc/varasm.c                                  |  4 ++++
>  2 files changed, 26 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/s390/litpool-str-1.c
>
> diff --git a/gcc/testsuite/gcc.target/s390/litpool-str-1.c b/gcc/testsuite/gcc.target/s390/litpool-str-1.c
> new file mode 100644
> index 0000000..cd921d2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/s390/litpool-str-1.c
> @@ -0,0 +1,22 @@
> +/* Make sure strings are recognized as being accessible through larl.
> +   This requires the symbol ref alignment properly propagated to
> +   encode_section_info.  */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-march=z900 -O2 -fpic" } */
> +
> +
> +extern void foo(const char*, const char*, const char*);
> +
> +void bar(int i)
> +{
> +  const char t1[10] = "test";
> +  const char t2[10] = "test2";
> +  const char t3[2][10] = {
> +       "foofoofoo",
> +       "barbarbar",
> +    };
> +  foo(t1, t2, t3[i]);
> +}
> +
> +/* { dg-final { scan-assembler-not "GOTOFF" } } */
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index 5b15847..68a7ba2 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -3296,6 +3296,10 @@ build_constant_desc (tree exp)
>    set_mem_attributes (rtl, exp, 1);
>    set_mem_alias_set (rtl, 0);
>
> +  /* Putting EXP into the literal pool might have imposed a larger
> +     alignment which should be visible in the RTX as well.  */
> +  set_mem_align (rtl, DECL_ALIGN (decl));
> +
>    /* We cannot share RTX'es in pool entries.
>       Mark this piece of RTL as required for unsharing.  */
>    RTX_FLAG (rtl, used) = 1;
> --
> 2.9.1
>

Re: [PATCH] varasm: Propagate litpool decl alignment to generated RTX.

[Andreas Krebbel]

On 12/20/2016 11:38 AM, Richard Biener wrote:
> On Fri, Dec 16, 2016 at 9:29 PM, Andreas Krebbel
> <krebbel@linux.vnet.ibm.com> wrote:
>> When pushing a value into the literal pool the resulting decl might
>> get a higher alignment than the original expression depending on how a
>> target defines CONSTANT_ALIGNMENT.  Generating an RTX for the constant
>> pool access we currently use the alignment from the original
>> expression.  Changed with the attached patch.
> 
> And it might be even smaller alignment...  or do we not allow that?
I did assume that this is not supposed to happen. Adding an assertion triggering in that case
survived bootstrap and testsuite. s390x only. It basically boils down to whether align_variable and
set_mem_attributes/get_object_alignment come to different conclusions about the alignment starting
at either the var decl or the original expression.

...
>> Bootstrapped and regtested on x86_64 and s390x.
>>
>> No regressions.
>>
>> Ok?
> 
> Ok.
> 
> Richard.

Ok for GCC 6 branch as well?

-Andreas-

Re: [PATCH] varasm: Propagate litpool decl alignment to generated RTX.

[Richard Biener]

On Thu, Dec 22, 2016 at 1:58 PM, Andreas Krebbel
<krebbel@linux.vnet.ibm.com> wrote:
> On 12/20/2016 11:38 AM, Richard Biener wrote:
>> On Fri, Dec 16, 2016 at 9:29 PM, Andreas Krebbel
>> <krebbel@linux.vnet.ibm.com> wrote:
>>> When pushing a value into the literal pool the resulting decl might
>>> get a higher alignment than the original expression depending on how a
>>> target defines CONSTANT_ALIGNMENT.  Generating an RTX for the constant
>>> pool access we currently use the alignment from the original
>>> expression.  Changed with the attached patch.
>>
>> And it might be even smaller alignment...  or do we not allow that?
> I did assume that this is not supposed to happen. Adding an assertion triggering in that case
> survived bootstrap and testsuite. s390x only. It basically boils down to whether align_variable and
> set_mem_attributes/get_object_alignment come to different conclusions about the alignment starting
> at either the var decl or the original expression.
>
> ...
>>> Bootstrapped and regtested on x86_64 and s390x.
>>>
>>> No regressions.
>>>
>>> Ok?
>>
>> Ok.
>>
>> Richard.
>
> Ok for GCC 6 branch as well?

Yes.

Richard.

> -Andreas-
>
>