RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[rohitarulraj]

[-- Attachment #1: Type: text/plain, Size: 3449 bytes --]

Hi,

I would like to resubmit these patches for comments. The previous detailed discussion is available in the below mentioned link.
https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01679.html
https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00489.html

The issue is still reproducible on GCC v4.8 branch.

I have tested both the attached patches with e500v2 tool chain on GCC v4.8 branch [rev 221667] and GCC trunk [rev 221310] with no regressions.

Patch1 [gcc.fix_pr60158_fixup_table_fsf_1]: Pass actual alignment value to output_constant_pool_2.
Patch2 [gcc.fix_pr60158_fixup_table_fsf_2]: Use the alignment data available in the first argument (constant_descriptor_rtx) of output_constant_pool_1.
        (Note: this generates ".align" directive twice).

Please let me know your comments.

Regards,
Rohit


-----Original Message-----
From: Dharmakan Rohit-B30502 
Sent: Wednesday, June 04, 2014 4:46 PM
To: gcc-patches@gcc.gnu.org
Subject: RE: [Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

Ping.
I have changed the subject line accordingly.

Regards,
Rohit

> -----Original Message-----
> From: David Edelsohn [mailto:dje.gcc@gmail.com]
> Sent: Thursday, May 08, 2014 9:28 PM
> To: Dharmakan Rohit-B30502; Jakub Jelinek; Richard Biener
> Cc: Alan Modra; gcc-patches@gcc.gnu.org; Wienskoski Edmar-RA8797
> Subject: Re: [Patch, PR 60158] Generate .fixup sections for 
> .data.rel.ro.local entries.
> 
> Rohit,
> 
> The subject line and thread may confuse people that this is a PowerPC- 
> specific issue. You need approval from a reviewer with authority over 
> varasm.c.
> 
> Thanks, David
> 
> On Thu, May 8, 2014 at 9:54 AM, rohitarulraj@freescale.com 
> <rohitarulraj@freescale.com> wrote:
> >> -----Original Message-----
> >> From: Alan Modra [mailto:amodra@gmail.com]
> >> Sent: Saturday, April 26, 2014 11:52 AM
> >> To: Dharmakan Rohit-B30502
> >> Cc: gcc-patches@gcc.gnu.org; dje.gcc@gmail.com; Wienskoski
> >> Edmar-RA8797
> >> Subject: Re: [Patch, PR 60158] Generate .fixup sections for 
> >> .data.rel.ro.local entries.
> >>
> >> On Fri, Apr 25, 2014 at 02:57:38PM +0000, 
> >> rohitarulraj@freescale.com
> >> wrote:
> >> > Source file: gcc-4.8.2/gcc/varasm.c @@ -7120,7 +7120,7 @@
> >> >        if (CONSTANT_POOL_ADDRESS_P (symbol))
> >> >         {
> >> >           desc = SYMBOL_REF_CONSTANT (symbol);
> >> >           output_constant_pool_1 (desc, 1);
> >> ------------- (A)
> >> >           offset += GET_MODE_SIZE (desc->mode);
> >>
> >> I think the reason 1 is passed here for align is that with 
> >> -fsection- anchors, in output_object_block we've already laid out 
> >> everything in the block, assigning offsets from the start of the 
> >> block.  Aligning shouldn't be necessary, because we've already done 
> >> that..  OTOH, it shouldn't hurt to align again.
> >>
> > Thanks. I have tested for both the cases on e500v2, e500mc, e5500,
> ppc64 (GCC v4.8.2 branch) with no regressions.
> >
> > Patch1 [gcc.fix_pr60158_fixup_table-fsf]: Pass actual alignment 
> > value
> to output_constant_pool_2.
> > Patch2 [gcc.fix_pr60158_fixup_table-fsf-2]: Use the alignment data
> available in the first argument (constant_descriptor_rtx) of 
> output_constant_pool_1.
> >         (Note: this generates ".align" directive twice).
> >
> > Is it ok to commit? Any comments?
> >
> > Regards,
> > Rohit
> >

[-- Attachment #2: gcc.fix_pr60158_fixup_table_trunk_fsf_1 --]
[-- Type: application/octet-stream, Size: 2919 bytes --]

Index: gcc/testsuite/gcc.target/powerpc/pr60158.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr60158.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr60158.c	(revision 0)
@@ -0,0 +1,89 @@
+/* { dg-do compile } */
+/* { dg-skip-if "not an SPE target" { ! powerpc_spe_nocache } { "*" } { "" } } */
+/* { dg-options "-mcpu=8548 -mno-spe -mfloat-gprs=double -Os -fdata-sections -fpic -mrelocatable" } */
+
+#define NULL 0
+int func (int val);
+void *func2 (void *ptr);
+
+static const char *ifs;
+static char map[256];
+
+typedef struct {
+/* None of these fields are used, but removing any
+   of them makes the problem go away.  */
+  char *data;
+  int length;
+  int maxlen;
+  int quote;
+} o_string;
+
+#define NULL_O_STRING {NULL,0,0,0}
+
+static int parse_stream (void *dest, void *ctx)
+{
+  int ch = func (0), m;
+
+  while (ch != -1) {
+    m = map[ch];
+    if (ch != '\n')
+    func2(dest);
+
+    ctx = func2 (ctx);
+    if (!func (0))
+      return 0;
+    if (m != ch) {
+      func2 ("htns");
+      break;
+    }
+  }
+  return -1;
+}
+
+static void mapset (const char *set, int code)
+{
+  const char *s;
+  for (s=set; *s; s++)  map[(int)*s] = code;
+}
+
+static void update_ifs_map(void)
+{
+  /* char *ifs and char map[256] are both globals.  */
+  ifs = func2 ("abc");
+  if (ifs == NULL) ifs="def";
+
+  func2 (map);
+  {
+    char subst[2] = {4, 0};
+    mapset (subst, 3);
+  }
+  mapset (";&|#", 1);
+}
+
+int parse_stream_outer (int flag)
+{
+  int blah;
+  o_string temp=NULL_O_STRING;
+  int rcode;
+
+  do {
+    update_ifs_map ();
+    func2 (&blah); /* a memory clobber works as well.  */
+    rcode = parse_stream (&temp, NULL);
+    func2 ("aoeu");
+    if (func (0) != 0) {
+      func2 (NULL);
+    }
+  } while (rcode != -1);
+  return 0;
+}
+
+/* { dg-final { if ![file exists pr60158.s] { fail "pr60158.c (compile)"; return; } } } */
+
+/* { dg-final { set c_rel [llength [grep pr60158.s \\.data\\.rel\\.ro\\.local]] } } */
+/* { dg-final { set c_fix [llength [grep pr60158.s \\.fixup]] } } */
+/* { dg-final { if [string match $c_rel $c_fix] \{	} } */
+/* { dg-final {     pass "pr60158.c (passed)"	} } */
+/* { dg-final { \} else \{	} } */
+/* { dg-final {     fail "pr60158.c (.fixup table entries not generated for .data.rel.ro.local section)"	} } */
+/* { dg-final { \}	} } */
Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(revision 221310)
+++ gcc/varasm.c	(working copy)
@@ -3944,7 +3944,7 @@ output_constant_pool_1 (struct constant_
   targetm.asm_out.internal_label (asm_out_file, "LC", desc->labelno);
 
   /* Output the data.  */
-  output_constant_pool_2 (desc->mode, x, align);
+  output_constant_pool_2 (desc->mode, x, desc->align);
 
   /* Make sure all constants in SECTION_MERGE and not SECTION_STRINGS
      sections have proper size.  */

[-- Attachment #3: gcc.fix_pr60158_fixup_table_trunk_fsf_2 --]
[-- Type: application/octet-stream, Size: 4390 bytes --]

Index: gcc/testsuite/gcc.target/powerpc/pr60158.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr60158.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr60158.c	(revision 0)
@@ -0,0 +1,89 @@
+/* { dg-do compile } */
+/* { dg-skip-if "not an SPE target" { ! powerpc_spe_nocache } { "*" } { "" } } */
+/* { dg-options "-mcpu=8548 -mno-spe -mfloat-gprs=double -Os -fdata-sections -fpic -mrelocatable" } */
+
+#define NULL 0
+int func (int val);
+void *func2 (void *ptr);
+
+static const char *ifs;
+static char map[256];
+
+typedef struct {
+/* None of these fields are used, but removing any
+   of them makes the problem go away.  */
+  char *data;
+  int length;
+  int maxlen;
+  int quote;
+} o_string;
+
+#define NULL_O_STRING {NULL,0,0,0}
+
+static int parse_stream (void *dest, void *ctx)
+{
+  int ch = func (0), m;
+
+  while (ch != -1) {
+    m = map[ch];
+    if (ch != '\n')
+    func2(dest);
+
+    ctx = func2 (ctx);
+    if (!func (0))
+      return 0;
+    if (m != ch) {
+      func2 ("htns");
+      break;
+    }
+  }
+  return -1;
+}
+
+static void mapset (const char *set, int code)
+{
+  const char *s;
+  for (s=set; *s; s++)  map[(int)*s] = code;
+}
+
+static void update_ifs_map(void)
+{
+  /* char *ifs and char map[256] are both globals.  */
+  ifs = func2 ("abc");
+  if (ifs == NULL) ifs="def";
+
+  func2 (map);
+  {
+    char subst[2] = {4, 0};
+    mapset (subst, 3);
+  }
+  mapset (";&|#", 1);
+}
+
+int parse_stream_outer (int flag)
+{
+  int blah;
+  o_string temp=NULL_O_STRING;
+  int rcode;
+
+  do {
+    update_ifs_map ();
+    func2 (&blah); /* a memory clobber works as well.  */
+    rcode = parse_stream (&temp, NULL);
+    func2 ("aoeu");
+    if (func (0) != 0) {
+      func2 (NULL);
+    }
+  } while (rcode != -1);
+  return 0;
+}
+
+/* { dg-final { if ![file exists pr60158.s] { fail "pr60158.c (compile)"; return; } } } */
+
+/* { dg-final { set c_rel [llength [grep pr60158.s \\.data\\.rel\\.ro\\.local]] } } */
+/* { dg-final { set c_fix [llength [grep pr60158.s \\.fixup]] } } */
+/* { dg-final { if [string match $c_rel $c_fix] \{	} } */
+/* { dg-final {     pass "pr60158.c (passed)"	} } */
+/* { dg-final { \} else \{	} } */
+/* { dg-final {     fail "pr60158.c (.fixup table entries not generated for .data.rel.ro.local section)"	} } */
+/* { dg-final { \}	} } */
Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(revision 221310)
+++ gcc/varasm.c	(working copy)
@@ -3896,8 +3896,7 @@ output_constant_pool_2 (machine_mode mod
    giving it ALIGN bits of alignment.  */
 
 static void
-output_constant_pool_1 (struct constant_descriptor_rtx *desc,
-			unsigned int align)
+output_constant_pool_1 (struct constant_descriptor_rtx *desc)
 {
   rtx x, tmp;
 
@@ -3935,23 +3934,23 @@ output_constant_pool_1 (struct constant_
 
 #ifdef ASM_OUTPUT_SPECIAL_POOL_ENTRY
   ASM_OUTPUT_SPECIAL_POOL_ENTRY (asm_out_file, x, desc->mode,
-				 align, desc->labelno, done);
+				 desc->align, desc->labelno, done);
 #endif
 
-  assemble_align (align);
+  assemble_align (desc->align);
 
   /* Output the label.  */
   targetm.asm_out.internal_label (asm_out_file, "LC", desc->labelno);
 
   /* Output the data.  */
-  output_constant_pool_2 (desc->mode, x, align);
+  output_constant_pool_2 (desc->mode, x, desc->align);
 
   /* Make sure all constants in SECTION_MERGE and not SECTION_STRINGS
      sections have proper size.  */
-  if (align > GET_MODE_BITSIZE (desc->mode)
+  if (desc->align > GET_MODE_BITSIZE (desc->mode)
       && in_section
       && (in_section->common.flags & SECTION_MERGE))
-    assemble_align (align);
+    assemble_align (desc->align);
 
 #ifdef ASM_OUTPUT_SPECIAL_POOL_ENTRY
  done:
@@ -4058,7 +4057,7 @@ output_constant_pool_contents (struct rt
 	  {
 	    switch_to_section (targetm.asm_out.select_rtx_section
 			       (desc->mode, desc->constant, desc->align));
-	    output_constant_pool_1 (desc, desc->align);
+	    output_constant_pool_1 (desc);
 	  }
       }
 }
@@ -7333,7 +7332,7 @@ output_object_block (struct object_block
       if (CONSTANT_POOL_ADDRESS_P (symbol))
 	{
 	  desc = SYMBOL_REF_CONSTANT (symbol);
-	  output_constant_pool_1 (desc, 1);
+	  output_constant_pool_1 (desc);
 	  offset += GET_MODE_SIZE (desc->mode);
 	}
       else if (TREE_CONSTANT_POOL_ADDRESS_P (symbol))

RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[rohitarulraj]

Ping.

-----Original Message-----
From: Dharmakan Rohit-B30502 
Sent: Friday, March 27, 2015 7:57 PM
To: gcc-patches@gcc.gnu.org; rguenther@suse.de; Jakub Jelinek
Cc: Alan Modra; David Edelsohn; Wienskoski Edmar-RA8797; Dharmakan Rohit-B30502
Subject: RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

Hi,

I would like to resubmit these patches for comments. The previous detailed discussion is available in the below mentioned link.
https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01679.html
https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00489.html

The issue is still reproducible on GCC v4.8 branch.

I have tested both the attached patches with e500v2 tool chain on GCC v4.8 branch [rev 221667] and GCC trunk [rev 221310] with no regressions.

Patch1 [gcc.fix_pr60158_fixup_table_fsf_1]: Pass actual alignment value to output_constant_pool_2.
Patch2 [gcc.fix_pr60158_fixup_table_fsf_2]: Use the alignment data available in the first argument (constant_descriptor_rtx) of output_constant_pool_1.
        (Note: this generates ".align" directive twice).

Please let me know your comments.

Regards,
Rohit


-----Original Message-----
From: Dharmakan Rohit-B30502
Sent: Wednesday, June 04, 2014 4:46 PM
To: gcc-patches@gcc.gnu.org
Subject: RE: [Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

Ping.
I have changed the subject line accordingly.

Regards,
Rohit

> -----Original Message-----
> From: David Edelsohn [mailto:dje.gcc@gmail.com]
> Sent: Thursday, May 08, 2014 9:28 PM
> To: Dharmakan Rohit-B30502; Jakub Jelinek; Richard Biener
> Cc: Alan Modra; gcc-patches@gcc.gnu.org; Wienskoski Edmar-RA8797
> Subject: Re: [Patch, PR 60158] Generate .fixup sections for 
> .data.rel.ro.local entries.
> 
> Rohit,
> 
> The subject line and thread may confuse people that this is a PowerPC- 
> specific issue. You need approval from a reviewer with authority over 
> varasm.c.
> 
> Thanks, David
> 
> On Thu, May 8, 2014 at 9:54 AM, rohitarulraj@freescale.com 
> <rohitarulraj@freescale.com> wrote:
> >> -----Original Message-----
> >> From: Alan Modra [mailto:amodra@gmail.com]
> >> Sent: Saturday, April 26, 2014 11:52 AM
> >> To: Dharmakan Rohit-B30502
> >> Cc: gcc-patches@gcc.gnu.org; dje.gcc@gmail.com; Wienskoski
> >> Edmar-RA8797
> >> Subject: Re: [Patch, PR 60158] Generate .fixup sections for 
> >> .data.rel.ro.local entries.
> >>
> >> On Fri, Apr 25, 2014 at 02:57:38PM +0000, 
> >> rohitarulraj@freescale.com
> >> wrote:
> >> > Source file: gcc-4.8.2/gcc/varasm.c @@ -7120,7 +7120,7 @@
> >> >        if (CONSTANT_POOL_ADDRESS_P (symbol))
> >> >         {
> >> >           desc = SYMBOL_REF_CONSTANT (symbol);
> >> >           output_constant_pool_1 (desc, 1);
> >> ------------- (A)
> >> >           offset += GET_MODE_SIZE (desc->mode);
> >>
> >> I think the reason 1 is passed here for align is that with
> >> -fsection- anchors, in output_object_block we've already laid out 
> >> everything in the block, assigning offsets from the start of the 
> >> block.  Aligning shouldn't be necessary, because we've already done 
> >> that..  OTOH, it shouldn't hurt to align again.
> >>
> > Thanks. I have tested for both the cases on e500v2, e500mc, e5500,
> ppc64 (GCC v4.8.2 branch) with no regressions.
> >
> > Patch1 [gcc.fix_pr60158_fixup_table-fsf]: Pass actual alignment 
> > value
> to output_constant_pool_2.
> > Patch2 [gcc.fix_pr60158_fixup_table-fsf-2]: Use the alignment data
> available in the first argument (constant_descriptor_rtx) of 
> output_constant_pool_1.
> >         (Note: this generates ".align" directive twice).
> >
> > Is it ok to commit? Any comments?
> >
> > Regards,
> > Rohit
> >

Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[Jeff Law]

On 04/28/2015 03:44 AM, rohitarulraj@freescale.com wrote:
> Ping.
>
> -----Original Message-----
> From: Dharmakan Rohit-B30502
> Sent: Friday, March 27, 2015 7:57 PM
> To: gcc-patches@gcc.gnu.org; rguenther@suse.de; Jakub Jelinek
> Cc: Alan Modra; David Edelsohn; Wienskoski Edmar-RA8797; Dharmakan Rohit-B30502
> Subject: RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2
>
> Hi,
>
> I would like to resubmit these patches for comments. The previous detailed discussion is available in the below mentioned link.
> https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01679.html
> https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00489.html
>
> The issue is still reproducible on GCC v4.8 branch.
>
> I have tested both the attached patches with e500v2 tool chain on GCC v4.8 branch [rev 221667] and GCC trunk [rev 221310] with no regressions.
>
> Patch1 [gcc.fix_pr60158_fixup_table_fsf_1]: Pass actual alignment value to output_constant_pool_2.
> Patch2 [gcc.fix_pr60158_fixup_table_fsf_2]: Use the alignment data available in the first argument (constant_descriptor_rtx) of output_constant_pool_1.
>          (Note: this generates ".align" directive twice).
Are you asking for both patches to be applied or just one?

Jeff

RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[rohitarulraj]

>-----Original Message-----
>From: Jeff Law [mailto:law@redhat.com] 
>Sent: Tuesday, April 28, 2015 11:48 PM
>To: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org; rguenther@suse.de; Jakub Jelinek
>Cc: Alan Modra; David Edelsohn; Wienskoski Edmar-RA8797
>Subject: Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2
>
>On 04/28/2015 03:44 AM, rohitarulraj@freescale.com wrote:
>> Ping.
>>
>> -----Original Message-----
>> From: Dharmakan Rohit-B30502
>> Sent: Friday, March 27, 2015 7:57 PM
>> To: gcc-patches@gcc.gnu.org; rguenther@suse.de; Jakub Jelinek
>> Cc: Alan Modra; David Edelsohn; Wienskoski Edmar-RA8797; Dharmakan Rohit-B30502
> >Subject: RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2
>>
>> Hi,
>>
>> I would like to resubmit these patches for comments. The previous detailed discussion is available in the below mentioned link.
>> https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01679.html
>> https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00489.html
>>
>> The issue is still reproducible on GCC v4.8 branch.
>>
>> I have tested both the attached patches with e500v2 tool chain on GCC v4.8 branch [rev 221667] and GCC trunk [rev 221310] with no regressions.
>>
>> Patch1 [gcc.fix_pr60158_fixup_table_fsf_1]: Pass actual alignment value to output_constant_pool_2.
>> Patch2 [gcc.fix_pr60158_fixup_table_fsf_2]: Use the alignment data available in the first argument (constant_descriptor_rtx) of output_constant_pool_1.
>>          (Note: this generates ".align" directive twice).
>Are you asking for both patches to be applied or just one?
Just one, needed your comments on which would be better.

Regards,
Rohit

Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[Jeff Law]

On 04/28/2015 12:38 PM, rohitarulraj@freescale.com wrote:
>
>
>> -----Original Message-----
>> From: Jeff Law [mailto:law@redhat.com]
>> Sent: Tuesday, April 28, 2015 11:48 PM
>> To: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org; rguenther@suse.de; Jakub Jelinek
>> Cc: Alan Modra; David Edelsohn; Wienskoski Edmar-RA8797
>> Subject: Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2
>>
>> On 04/28/2015 03:44 AM, rohitarulraj@freescale.com wrote:
>>> Ping.
>>>
>>> -----Original Message-----
>>> From: Dharmakan Rohit-B30502
>>> Sent: Friday, March 27, 2015 7:57 PM
>>> To: gcc-patches@gcc.gnu.org; rguenther@suse.de; Jakub Jelinek
>>> Cc: Alan Modra; David Edelsohn; Wienskoski Edmar-RA8797; Dharmakan Rohit-B30502
>>> Subject: RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2
>>>
>>> Hi,
>>>
>>> I would like to resubmit these patches for comments. The previous detailed discussion is available in the below mentioned link.
>>> https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01679.html
>>> https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00489.html
>>>
>>> The issue is still reproducible on GCC v4.8 branch.
>>>
>>> I have tested both the attached patches with e500v2 tool chain on GCC v4.8 branch [rev 221667] and GCC trunk [rev 221310] with no regressions.
>>>
>>> Patch1 [gcc.fix_pr60158_fixup_table_fsf_1]: Pass actual alignment value to output_constant_pool_2.
>>> Patch2 [gcc.fix_pr60158_fixup_table_fsf_2]: Use the alignment data available in the first argument (constant_descriptor_rtx) of output_constant_pool_1.
>>>           (Note: this generates ".align" directive twice).
>> Are you asking for both patches to be applied or just one?
> Just one, needed your comments on which would be better.
Just wanted to be sure.  Particularly since I could make a case for 
either or both.

I think this is the better patch:


https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00489/gcc.fix_pr60158_fixup_table-fsf

The change I would request would be to add some comments.  So before 
this code:

output_constant_pool_1 (desc, 1);

A comment about why passing "1" for the alignment is OK here (because 
all the data is already aligned, so no need to realign it).

And for this change:

-  output_constant_pool_2 (desc->mode, x, align);
+  output_constant_pool_2 (desc->mode, x, desc->align);

I would suggest a comment why we're using desc->align rather than align. 
  You'll probably want/need to refer back to the call where "1" is 
passed for the alignment in that comment.


With those two comments added, and a fresh bootstrap & regression test 
on the trunk, it'll be good to go.

jeff

RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[rohitarulraj]

[-- Attachment #1: Type: text/plain, Size: 3455 bytes --]

> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Wednesday, April 29, 2015 4:01 AM
> To: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org;
> rguenther@suse.de; Jakub Jelinek
> Cc: Alan Modra; David Edelsohn; Wienskoski Edmar-RA8797
> Subject: Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value
> to output_constant_pool_2
> 
> On 04/28/2015 12:38 PM, rohitarulraj@freescale.com wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jeff Law [mailto:law@redhat.com]
> >> Sent: Tuesday, April 28, 2015 11:48 PM
> >> To: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org;
> >> rguenther@suse.de; Jakub Jelinek
> >> Cc: Alan Modra; David Edelsohn; Wienskoski Edmar-RA8797
> >> Subject: Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual
> >> alignment value to output_constant_pool_2
> >>
> >> On 04/28/2015 03:44 AM, rohitarulraj@freescale.com wrote:
> >>> Ping.
> >>>
> >>> -----Original Message-----
> >>> From: Dharmakan Rohit-B30502
> >>> Sent: Friday, March 27, 2015 7:57 PM
> >>> To: gcc-patches@gcc.gnu.org; rguenther@suse.de; Jakub Jelinek
> >>> Cc: Alan Modra; David Edelsohn; Wienskoski Edmar-RA8797; Dharmakan
> >>> Rohit-B30502
> >>> Subject: RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual
> >>> alignment value to output_constant_pool_2
> >>>
> >>> Hi,
> >>>
> >>> I would like to resubmit these patches for comments. The previous
> detailed discussion is available in the below mentioned link.
> >>> https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01679.html
> >>> https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00489.html
> >>>
> >>> The issue is still reproducible on GCC v4.8 branch.
> >>>
> >>> I have tested both the attached patches with e500v2 tool chain on GCC
> v4.8 branch [rev 221667] and GCC trunk [rev 221310] with no regressions.
> >>>
> >>> Patch1 [gcc.fix_pr60158_fixup_table_fsf_1]: Pass actual alignment value
> to output_constant_pool_2.
> >>> Patch2 [gcc.fix_pr60158_fixup_table_fsf_2]: Use the alignment data
> available in the first argument (constant_descriptor_rtx) of
> output_constant_pool_1.
> >>>           (Note: this generates ".align" directive twice).
> >> Are you asking for both patches to be applied or just one?
> > Just one, needed your comments on which would be better.
> Just wanted to be sure.  Particularly since I could make a case for either or
> both.
> 
> I think this is the better patch:
> 
> 
> https://gcc.gnu.org/ml/gcc-patches/2014-
> 05/msg00489/gcc.fix_pr60158_fixup_table-fsf
> 
> The change I would request would be to add some comments.  So before
> this code:
> 
> output_constant_pool_1 (desc, 1);
> 
> A comment about why passing "1" for the alignment is OK here (because all
> the data is already aligned, so no need to realign it).
> 
> And for this change:
> 
> -  output_constant_pool_2 (desc->mode, x, align);
> +  output_constant_pool_2 (desc->mode, x, desc->align);
> 
> I would suggest a comment why we're using desc->align rather than align.
>   You'll probably want/need to refer back to the call where "1" is passed for
> the alignment in that comment.
> 
> 
> With those two comments added, and a fresh bootstrap & regression test
> on the trunk, it'll be good to go.
> 

Jeff, I have made the changes as per your comments and attached the patch.
If the patch is OK, I will proceed with the regression tests.

Regards,
Rohit


[-- Attachment #2: gcc.fix_pr60158_fixup_table_trunk_fsf --]
[-- Type: application/octet-stream, Size: 3603 bytes --]

Index: gcc/testsuite/gcc.target/powerpc/pr60158.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr60158.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr60158.c	(revision 0)
@@ -0,0 +1,89 @@
+/* { dg-do compile } */
+/* { dg-skip-if "not an SPE target" { ! powerpc_spe_nocache } { "*" } { "" } } */
+/* { dg-options "-mcpu=8548 -mno-spe -mfloat-gprs=double -Os -fdata-sections -fpic -mrelocatable" } */
+
+#define NULL 0
+int func (int val);
+void *func2 (void *ptr);
+
+static const char *ifs;
+static char map[256];
+
+typedef struct {
+/* None of these fields are used, but removing any
+   of them makes the problem go away.  */
+  char *data;
+  int length;
+  int maxlen;
+  int quote;
+} o_string;
+
+#define NULL_O_STRING {NULL,0,0,0}
+
+static int parse_stream (void *dest, void *ctx)
+{
+  int ch = func (0), m;
+
+  while (ch != -1) {
+    m = map[ch];
+    if (ch != '\n')
+    func2(dest);
+
+    ctx = func2 (ctx);
+    if (!func (0))
+      return 0;
+    if (m != ch) {
+      func2 ("htns");
+      break;
+    }
+  }
+  return -1;
+}
+
+static void mapset (const char *set, int code)
+{
+  const char *s;
+  for (s=set; *s; s++)  map[(int)*s] = code;
+}
+
+static void update_ifs_map(void)
+{
+  /* char *ifs and char map[256] are both globals.  */
+  ifs = func2 ("abc");
+  if (ifs == NULL) ifs="def";
+
+  func2 (map);
+  {
+    char subst[2] = {4, 0};
+    mapset (subst, 3);
+  }
+  mapset (";&|#", 1);
+}
+
+int parse_stream_outer (int flag)
+{
+  int blah;
+  o_string temp=NULL_O_STRING;
+  int rcode;
+
+  do {
+    update_ifs_map ();
+    func2 (&blah); /* a memory clobber works as well.  */
+    rcode = parse_stream (&temp, NULL);
+    func2 ("aoeu");
+    if (func (0) != 0) {
+      func2 (NULL);
+    }
+  } while (rcode != -1);
+  return 0;
+}
+
+/* { dg-final { if ![file exists pr60158.s] { fail "pr60158.c (compile)"; return; } } } */
+
+/* { dg-final { set c_rel [llength [grep pr60158.s \\.data\\.rel\\.ro\\.local]] } } */
+/* { dg-final { set c_fix [llength [grep pr60158.s \\.fixup]] } } */
+/* { dg-final { if [string match $c_rel $c_fix] \{	} } */
+/* { dg-final {     pass "pr60158.c (passed)"	} } */
+/* { dg-final { \} else \{	} } */
+/* { dg-final {     fail "pr60158.c (.fixup table entries not generated for .data.rel.ro.local section)"	} } */
+/* { dg-final { \}	} } */
Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(revision 222452)
+++ gcc/varasm.c	(working copy)
@@ -3942,8 +3942,12 @@ output_constant_pool_1 (struct constant_
   /* Output the label.  */
   targetm.asm_out.internal_label (asm_out_file, "LC", desc->labelno);
 
-  /* Output the data.  */
-  output_constant_pool_2 (desc->mode, x, align);
+  /* Output the data.
+     Pass actual alignment value while emitting string constant to asm code
+     as function 'output_constant_pool_1' explicitly passes the alignment as 1
+     assuming that the data is already aligned which prevents the generation 
+     of fix-up table entries.  */
+  output_constant_pool_2 (desc->mode, x, desc->align);
 
   /* Make sure all constants in SECTION_MERGE and not SECTION_STRINGS
      sections have proper size.  */
@@ -7355,6 +7359,8 @@ output_object_block (struct object_block
       if (CONSTANT_POOL_ADDRESS_P (symbol))
 	{
 	  desc = SYMBOL_REF_CONSTANT (symbol);
+	  /* Pass 1 for align as we have already laid out everything in the block.
+	     So aligning shouldn't be necessary.  */
 	  output_constant_pool_1 (desc, 1);
 	  offset += GET_MODE_SIZE (desc->mode);
 	}

Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[Jeff Law]

On 04/29/2015 04:30 AM, rohitarulraj@freescale.com wrote:
>>
>
> Jeff, I have made the changes as per your comments and attached the patch.
> If the patch is OK, I will proceed with the regression tests.
This patch refers back to 60158 and based on what I see in 60158, it 
appears I should be looking for a .data.rel.ro.local section which 
contains the address of a string constant.  But the constants are being 
put into .rodata.str1.4.  And if the issue is we're putting bits into 
the wrong section and don't have an appropriate .fixup section, then 
ISTM that the test should be compiled, then objdump used to verify the 
sections and/or relocations.

An additional concern is that I get the same code for the included 
testcase with or without your changes.  This is with a 
powerpc-softfloat-linux-gnuspe configured compiler -- which matches what 
I saw in pr 60158.

So while the patch seems reasonable, I'm concerned that I've been unable 
to show it changing anything.

Thoughts?

Jeff

RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[rohitarulraj]

> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Thursday, April 30, 2015 8:32 PM
> To: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org;
> rguenther@suse.de; Jakub Jelinek
> Cc: Alan Modra; David Edelsohn; Wienskoski Edmar-RA8797
> Subject: Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value
> to output_constant_pool_2
> 
> On 04/29/2015 04:30 AM, rohitarulraj@freescale.com wrote:
> >>
> >
> > Jeff, I have made the changes as per your comments and attached the
> patch.
> > If the patch is OK, I will proceed with the regression tests.
> This patch refers back to 60158 and based on what I see in 60158, it appears I
> should be looking for a .data.rel.ro.local section which contains the address
> of a string constant.  But the constants are being put into .rodata.str1.4.  And
> if the issue is we're putting bits into the wrong section and don't have an
> appropriate .fixup section, then ISTM that the test should be compiled, then
> objdump used to verify the sections and/or relocations.
> 
> An additional concern is that I get the same code for the included testcase
> with or without your changes.  This is with a powerpc-softfloat-linux-gnuspe
> configured compiler -- which matches what I saw in pr 60158.
> 
> So while the patch seems reasonable, I'm concerned that I've been unable to
> show it changing anything.
> 
> Thoughts?
> 

Jeff, the issue is still reproducible with GCC v4.8 branch but not with GCC v4.9 or trunk.

Regards,
Rohit

Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[Jeff Law]

On 04/30/2015 09:34 AM, rohitarulraj@freescale.com wrote:
>
>
>> -----Original Message-----
>> From: Jeff Law [mailto:law@redhat.com]
>> Sent: Thursday, April 30, 2015 8:32 PM
>> To: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org;
>> rguenther@suse.de; Jakub Jelinek
>> Cc: Alan Modra; David Edelsohn; Wienskoski Edmar-RA8797
>> Subject: Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value
>> to output_constant_pool_2
>>
>> On 04/29/2015 04:30 AM, rohitarulraj@freescale.com wrote:
>>>>
>>>
>>> Jeff, I have made the changes as per your comments and attached the
>> patch.
>>> If the patch is OK, I will proceed with the regression tests.
>> This patch refers back to 60158 and based on what I see in 60158, it appears I
>> should be looking for a .data.rel.ro.local section which contains the address
>> of a string constant.  But the constants are being put into .rodata.str1.4.  And
>> if the issue is we're putting bits into the wrong section and don't have an
>> appropriate .fixup section, then ISTM that the test should be compiled, then
>> objdump used to verify the sections and/or relocations.
>>
>> An additional concern is that I get the same code for the included testcase
>> with or without your changes.  This is with a powerpc-softfloat-linux-gnuspe
>> configured compiler -- which matches what I saw in pr 60158.
>>
>> So while the patch seems reasonable, I'm concerned that I've been unable to
>> show it changing anything.
>>
>> Thoughts?
>>
>
> Jeff, the issue is still reproducible with GCC v4.8 branch but not with GCC v4.9 or trunk.
Was it fixed by some other approach or has the bug gone latent? 
Obviously if the former, then the patch is only relevant to gcc-4.8 if 
the latter, then we'll still want to get it fixed on the trunk and 
possibly in the release branches.

Can you please investigate if the bug has been fixed by other means or 
if it's just gone latent on the trunk?

Thanks,
jeff

RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[rohitarulraj]

> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Thursday, April 30, 2015 9:14 PM
> On 04/30/2015 09:34 AM, rohitarulraj@freescale.com wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jeff Law [mailto:law@redhat.com]
> >> Sent: Thursday, April 30, 2015 8:32 PM
> >> To: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org;
> >> rguenther@suse.de; Jakub Jelinek
> >> Cc: Alan Modra; David Edelsohn; Wienskoski Edmar-RA8797
> >> Subject: Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual
> >> alignment value to output_constant_pool_2
> >>
> >> On 04/29/2015 04:30 AM, rohitarulraj@freescale.com wrote:
> >>>>
> >>>
> >>> Jeff, I have made the changes as per your comments and attached the
> >> patch.
> >>> If the patch is OK, I will proceed with the regression tests.
> >> This patch refers back to 60158 and based on what I see in 60158, it
> >> appears I should be looking for a .data.rel.ro.local section which
> >> contains the address of a string constant.  But the constants are
> >> being put into .rodata.str1.4.  And if the issue is we're putting
> >> bits into the wrong section and don't have an appropriate .fixup
> >> section, then ISTM that the test should be compiled, then objdump used
> to verify the sections and/or relocations.
> >>
> >> An additional concern is that I get the same code for the included
> >> testcase with or without your changes.  This is with a
> >> powerpc-softfloat-linux-gnuspe configured compiler -- which matches
> what I saw in pr 60158.
> >>
> >> So while the patch seems reasonable, I'm concerned that I've been
> >> unable to show it changing anything.
> >>
> >> Thoughts?
> >>
> >
> > Jeff, the issue is still reproducible with GCC v4.8 branch but not with GCC
> v4.9 or trunk.
> Was it fixed by some other approach or has the bug gone latent?
> Obviously if the former, then the patch is only relevant to gcc-4.8 if the latter,
> then we'll still want to get it fixed on the trunk and possibly in the release
> branches.
> 
> Can you please investigate if the bug has been fixed by other means or if it's
> just gone latent on the trunk?

Jeff, 

Just to summarize:
By default in GCC v4.7.x, all the constants are put into '.rodata.str1.4' section. In GCC v4.8.x from r192719 onwards, one of the move instruction of the string constant ".LC0" is getting spilled. The reload pass, for any constants that aren't allowed and can't be reloaded in to registers tries to change them into memory references. Then while emitting that string constant to asm code (A:varasm.c: output_constant_pool_1), it explicitly passes the alignment as 1 which prevents the generation of fix-up table entries in  'B: rs6000.c:rs6000_assemble_integer' because the data is considered unaligned now.

The bug seems to have gone latent with an unrelated trunk commit r204695 [* tree-ssa-loop-ivopts.c (force_expr_to_var_cost): Refactor the code. Handle type conversion.]. This commit chooses different spill candidates hence all the string constants are being put in to '.rodata.str1.4´section.

The check I had in the test case is that if there is a '.data.rel.ro.local', then there should be '.fixup' section generated.

Please let me know if you need any other details.

Regards,
Rohit

Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[Jeff Law]

On 05/05/2015 01:59 AM, rohitarulraj@freescale.com wrote:
>>> Jeff, the issue is still reproducible with GCC v4.8 branch but
>>> not with GCC
>> v4.9 or trunk. Was it fixed by some other approach or has the bug
>> gone latent? Obviously if the former, then the patch is only
>> relevant to gcc-4.8 if the latter, then we'll still want to get it
>> fixed on the trunk and possibly in the release branches.
>>
>> Can you please investigate if the bug has been fixed by other means
>> or if it's just gone latent on the trunk?
>
> Jeff,
>
> Just to summarize: By default in GCC v4.7.x, all the constants are
> put into '.rodata.str1.4' section. In GCC v4.8.x from r192719
> onwards, one of the move instruction of the string constant ".LC0" is
> getting spilled. The reload pass, for any constants that aren't
> allowed and can't be reloaded in to registers tries to change them
> into memory references. Then while emitting that string constant to
> asm code (A:varasm.c: output_constant_pool_1), it explicitly passes
> the alignment as 1 which prevents the generation of fix-up table
> entries in  'B: rs6000.c:rs6000_assemble_integer' because the data is
> considered unaligned now.
>
> The bug seems to have gone latent with an unrelated trunk commit
> r204695 [* tree-ssa-loop-ivopts.c (force_expr_to_var_cost): Refactor
> the code. Handle type conversion.]. This commit chooses different
> spill candidates hence all the string constants are being put in to
> '.rodata.str1.4´section.
>
> The check I had in the test case is that if there is a
> '.data.rel.ro.local', then there should be '.fixup' section
> generated.
>
> Please let me know if you need any other details.
Thanks.  Even though I wasn't able to trigger the bug with the testcase 
from 65018, I went ahead and committed this patch to the trunk.  It 
can't hurt and it's the right thing to do.

Thanks for your patience,

Jeff

RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[Dharmakan Rohit Arul Raj]

> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Friday, May 15, 2015 10:30 AM
> > Just to summarize: By default in GCC v4.7.x, all the constants are put
> > into '.rodata.str1.4' section. In GCC v4.8.x from r192719 onwards, one
> > of the move instruction of the string constant ".LC0" is getting
> > spilled. The reload pass, for any constants that aren't allowed and
> > can't be reloaded in to registers tries to change them into memory
> > references. Then while emitting that string constant to asm code
> > (A:varasm.c: output_constant_pool_1), it explicitly passes the
> > alignment as 1 which prevents the generation of fix-up table entries
> > in  'B: rs6000.c:rs6000_assemble_integer' because the data is
> > considered unaligned now.
> >
> > The bug seems to have gone latent with an unrelated trunk commit
> > r204695 [* tree-ssa-loop-ivopts.c (force_expr_to_var_cost): Refactor
> > the code. Handle type conversion.]. This commit chooses different
> > spill candidates hence all the string constants are being put in to
> > '.rodata.str1.4´section.
> >
> > The check I had in the test case is that if there is a
> > '.data.rel.ro.local', then there should be '.fixup' section generated.
> >
> > Please let me know if you need any other details.
> Thanks.  Even though I wasn't able to trigger the bug with the testcase from
> 65018, I went ahead and committed this patch to the trunk.  It can't hurt and
> it's the right thing to do.
> 
> Thanks for your patience,
> 

Jeff, Thanks for checking-in the changes.
Can we apply the patch to GCC v4.8 and GCC v4.9 branch as well?

As mentioned earlier, the issue is still reproducible in GCC v4.8 branch [r223205] but not in GCC v4.9 branch.

Regards,
Rohit

Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[Jeff Law]

On 05/15/2015 04:37 AM, Dharmakan Rohit Arul Raj wrote:
>
>> -----Original Message-----
>> From: Jeff Law [mailto:law@redhat.com]
>> Sent: Friday, May 15, 2015 10:30 AM
>>> Just to summarize: By default in GCC v4.7.x, all the constants are put
>>> into '.rodata.str1.4' section. In GCC v4.8.x from r192719 onwards, one
>>> of the move instruction of the string constant ".LC0" is getting
>>> spilled. The reload pass, for any constants that aren't allowed and
>>> can't be reloaded in to registers tries to change them into memory
>>> references. Then while emitting that string constant to asm code
>>> (A:varasm.c: output_constant_pool_1), it explicitly passes the
>>> alignment as 1 which prevents the generation of fix-up table entries
>>> in  'B: rs6000.c:rs6000_assemble_integer' because the data is
>>> considered unaligned now.
>>>
>>> The bug seems to have gone latent with an unrelated trunk commit
>>> r204695 [* tree-ssa-loop-ivopts.c (force_expr_to_var_cost): Refactor
>>> the code. Handle type conversion.]. This commit chooses different
>>> spill candidates hence all the string constants are being put in to
>>> '.rodata.str1.4´section.
>>>
>>> The check I had in the test case is that if there is a
>>> '.data.rel.ro.local', then there should be '.fixup' section generated.
>>>
>>> Please let me know if you need any other details.
>> Thanks.  Even though I wasn't able to trigger the bug with the testcase from
>> 65018, I went ahead and committed this patch to the trunk.  It can't hurt and
>> it's the right thing to do.
>>
>> Thanks for your patience,
>>
>
> Jeff, Thanks for checking-in the changes.
> Can we apply the patch to GCC v4.8 and GCC v4.9 branch as well?
That's up to the branch maintainers.  I'd think it's safe, but it's 
ultimately their call.

jeff

Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[Richard Biener]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2243 bytes --]

On Fri, 15 May 2015, Jeff Law wrote:

> On 05/15/2015 04:37 AM, Dharmakan Rohit Arul Raj wrote:
> > 
> > > -----Original Message-----
> > > From: Jeff Law [mailto:law@redhat.com]
> > > Sent: Friday, May 15, 2015 10:30 AM
> > > > Just to summarize: By default in GCC v4.7.x, all the constants are put
> > > > into '.rodata.str1.4' section. In GCC v4.8.x from r192719 onwards, one
> > > > of the move instruction of the string constant ".LC0" is getting
> > > > spilled. The reload pass, for any constants that aren't allowed and
> > > > can't be reloaded in to registers tries to change them into memory
> > > > references. Then while emitting that string constant to asm code
> > > > (A:varasm.c: output_constant_pool_1), it explicitly passes the
> > > > alignment as 1 which prevents the generation of fix-up table entries
> > > > in  'B: rs6000.c:rs6000_assemble_integer' because the data is
> > > > considered unaligned now.
> > > > 
> > > > The bug seems to have gone latent with an unrelated trunk commit
> > > > r204695 [* tree-ssa-loop-ivopts.c (force_expr_to_var_cost): Refactor
> > > > the code. Handle type conversion.]. This commit chooses different
> > > > spill candidates hence all the string constants are being put in to
> > > > '.rodata.str1.4´section.
> > > > 
> > > > The check I had in the test case is that if there is a
> > > > '.data.rel.ro.local', then there should be '.fixup' section generated.
> > > > 
> > > > Please let me know if you need any other details.
> > > Thanks.  Even though I wasn't able to trigger the bug with the testcase
> > > from
> > > 65018, I went ahead and committed this patch to the trunk.  It can't hurt
> > > and
> > > it's the right thing to do.
> > > 
> > > Thanks for your patience,
> > > 
> > 
> > Jeff, Thanks for checking-in the changes.
> > Can we apply the patch to GCC v4.8 and GCC v4.9 branch as well?
> That's up to the branch maintainers.  I'd think it's safe, but it's ultimately
> their call.

If it's a regression or a wrong-code issue then it's ok to backport given
the patch is safe, of course.

Richard.

> jeff
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)

RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[Dharmakan Rohit Arul Raj]

> -----Original Message-----
> From: Richard Biener [mailto:rguenther@suse.de]
> Sent: Monday, May 18, 2015 1:06 PM
> To: Jeff Law
> Cc: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org; Jakub Jelinek; Alan
> Modra; David Edelsohn; Wienskoski Edmar-RA8797
> Subject: Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value
> to output_constant_pool_2
> 
> On Fri, 15 May 2015, Jeff Law wrote:
> 
> > On 05/15/2015 04:37 AM, Dharmakan Rohit Arul Raj wrote:
> > >
> > > > -----Original Message-----
> > > > From: Jeff Law [mailto:law@redhat.com]
> > > > Sent: Friday, May 15, 2015 10:30 AM
> > > > > Just to summarize: By default in GCC v4.7.x, all the constants
> > > > > are put into '.rodata.str1.4' section. In GCC v4.8.x from
> > > > > r192719 onwards, one of the move instruction of the string
> > > > > constant ".LC0" is getting spilled. The reload pass, for any
> > > > > constants that aren't allowed and can't be reloaded in to
> > > > > registers tries to change them into memory references. Then
> > > > > while emitting that string constant to asm code
> > > > > (A:varasm.c: output_constant_pool_1), it explicitly passes the
> > > > > alignment as 1 which prevents the generation of fix-up table
> > > > > entries in  'B: rs6000.c:rs6000_assemble_integer' because the
> > > > > data is considered unaligned now.
> > > > >
> > > > > The bug seems to have gone latent with an unrelated trunk commit
> > > > > r204695 [* tree-ssa-loop-ivopts.c (force_expr_to_var_cost):
> > > > > Refactor the code. Handle type conversion.]. This commit chooses
> > > > > different spill candidates hence all the string constants are
> > > > > being put in to '.rodata.str1.4´section.
> > > > >
> > > > > The check I had in the test case is that if there is a
> > > > > '.data.rel.ro.local', then there should be '.fixup' section generated.
> > > > >
> > > > > Please let me know if you need any other details.
> > > > Thanks.  Even though I wasn't able to trigger the bug with the
> > > > testcase from 65018, I went ahead and committed this patch to the
> > > > trunk.  It can't hurt and it's the right thing to do.
> > > >
> > > > Thanks for your patience,
> > > >
> > >
> > > Jeff, Thanks for checking-in the changes.
> > > Can we apply the patch to GCC v4.8 and GCC v4.9 branch as well?
> > That's up to the branch maintainers.  I'd think it's safe, but it's
> > ultimately their call.
> 
> If it's a regression or a wrong-code issue then it's ok to backport given the
> patch is safe, of course.

Just to confirm, can we backport the patch to GCC v4.8 & GCC v4.9 branch?

Regards,
Rohit
 

RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2

[Richard Biener]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2824 bytes --]

On Mon, 25 May 2015, Dharmakan Rohit Arul Raj wrote:

> > -----Original Message-----
> > From: Richard Biener [mailto:rguenther@suse.de]
> > Sent: Monday, May 18, 2015 1:06 PM
> > To: Jeff Law
> > Cc: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org; Jakub Jelinek; Alan
> > Modra; David Edelsohn; Wienskoski Edmar-RA8797
> > Subject: Re: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value
> > to output_constant_pool_2
> > 
> > On Fri, 15 May 2015, Jeff Law wrote:
> > 
> > > On 05/15/2015 04:37 AM, Dharmakan Rohit Arul Raj wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Jeff Law [mailto:law@redhat.com]
> > > > > Sent: Friday, May 15, 2015 10:30 AM
> > > > > > Just to summarize: By default in GCC v4.7.x, all the constants
> > > > > > are put into '.rodata.str1.4' section. In GCC v4.8.x from
> > > > > > r192719 onwards, one of the move instruction of the string
> > > > > > constant ".LC0" is getting spilled. The reload pass, for any
> > > > > > constants that aren't allowed and can't be reloaded in to
> > > > > > registers tries to change them into memory references. Then
> > > > > > while emitting that string constant to asm code
> > > > > > (A:varasm.c: output_constant_pool_1), it explicitly passes the
> > > > > > alignment as 1 which prevents the generation of fix-up table
> > > > > > entries in  'B: rs6000.c:rs6000_assemble_integer' because the
> > > > > > data is considered unaligned now.
> > > > > >
> > > > > > The bug seems to have gone latent with an unrelated trunk commit
> > > > > > r204695 [* tree-ssa-loop-ivopts.c (force_expr_to_var_cost):
> > > > > > Refactor the code. Handle type conversion.]. This commit chooses
> > > > > > different spill candidates hence all the string constants are
> > > > > > being put in to '.rodata.str1.4´section.
> > > > > >
> > > > > > The check I had in the test case is that if there is a
> > > > > > '.data.rel.ro.local', then there should be '.fixup' section generated.
> > > > > >
> > > > > > Please let me know if you need any other details.
> > > > > Thanks.  Even though I wasn't able to trigger the bug with the
> > > > > testcase from 65018, I went ahead and committed this patch to the
> > > > > trunk.  It can't hurt and it's the right thing to do.
> > > > >
> > > > > Thanks for your patience,
> > > > >
> > > >
> > > > Jeff, Thanks for checking-in the changes.
> > > > Can we apply the patch to GCC v4.8 and GCC v4.9 branch as well?
> > > That's up to the branch maintainers.  I'd think it's safe, but it's
> > > ultimately their call.
> > 
> > If it's a regression or a wrong-code issue then it's ok to backport given the
> > patch is safe, of course.
> 
> Just to confirm, can we backport the patch to GCC v4.8 & GCC v4.9 branch?

I've already said what I had to say.  I didn't look at the patch.

Richard.