RE: [RFC: Patch, PR 60102] [4.9/4.10 Regression] powerpc fp-bit ices@dwf_regno
> From: Dharmakan Rohit-B30502
> Sent: Monday, September 29, 2014 3:54 PM
>
> > From: Ulrich Weigand [mailto:uweig...@de.ibm.com] Maciej W. Rozycki
> > wrote:
> > > On Mon, 4 Aug 2014, Edmar wrote:
> > >
> > > > Committed on trunk, revision 213596 Committed on 4.9 branch,
> > > > revision 213597
> > >
> > > This change regressed GDB for e500v2 multilibs, presumably because
> > > it does not understand the new DWARF register numbers and does not
> > > know how to map them to hardware registers.
> >
> > As I understand it, the change was supposed to only affect GCC
> > internals, all externally generated debug info was supposed to remain
> unchanged.
> >
> > If there are changes in debug info, something must have gone wrong.
>
> Let me check if I can track this down.
Maciej/Ulrich,
I was able to narrow down the issue.
Debug info generated with the current patch:
<2><334>: Abbrev Number: 10 (DW_TAG_formal_parameter)
<335> DW_AT_name: u
<337> DW_AT_decl_file : 1
<338> DW_AT_decl_line : 51
<339> DW_AT_type: <0x357>
<33d> DW_AT_location: 7 byte block: 90 7d 93 4 58 93 4
(DW_OP_regx: 125 (r125); DW_OP_piece: 4; DW_OP_reg8 (r8); DW_OP_piece: 4)
Expected debug info:
<2><334>: Abbrev Number: 10 (DW_TAG_formal_parameter)
<335> DW_AT_name: u
<337> DW_AT_decl_file : 1
<338> DW_AT_decl_line : 51
<339> DW_AT_type: <0x359>
<33d> DW_AT_location: 8 byte block: 90 b8 9 93 4 58 93 4
(DW_OP_regx: 1208 (r1208); DW_OP_piece: 4; DW_OP_reg8 (r8); DW_OP_piece: 4)
While emitting the location descriptors of multiple registers (SPE high/low
part) individually, the GCC hard register number is converted in to DWARF
register number using "dbx_reg_number" [Statement "A", "B" & "C" below].
File1: gcc-4.9/gcc/config/rs6000/rs6000.h
=
...
/* Use standard DWARF numbering for DWARF debugging information. */
#define DBX_REGISTER_NUMBER(REGNO) rs6000_dbx_register_number (REGNO)
-(A)
...
File2: gcc-4.9/gcc/dwarf2out.c
=
/* Given an RTL of a register, return a location descriptor that
designates a value that spans more than one register. */
static dw_loc_descr_ref
multiple_reg_loc_descriptor (rtx rtl, rtx regs,
enum var_init_status initialized)
{
...
/* Now onto stupid register sets in non contiguous locations. */
for (i = 0; i < XVECLEN (regs, 0); ++i)
{
dw_loc_descr_ref t;
t = one_reg_loc_descriptor (dbx_reg_number (XVECEXP (regs, 0, i)),
VAR_INIT_STATUS_INITIALIZED); ---(B)
...
}
.
}
static unsigned int
dbx_reg_number (const_rtx rtl)
{
regno = DBX_REGISTER_NUMBER (regno);
-(C)
gcc_assert (regno != INVALID_REGNUM);
return regno;
}
File3:gcc-4.9/gcc/config/rs6000/sysv4.h
===
/* This target uses the sysv4.opt file. */
#define TARGET_USES_SYSV4_OPT 1
#undef DBX_REGISTER_NUMBER
--(D)
But statement "C" macro "DBX_REGISTER_NUMBER" gets undefined by statement "D"
hence the GCC hard register number gets emitted in the debug info instead of
DWARF register number.
Previously, without this patch for SPE high registers the GCC hard register
number was same as the DWARF register number (1200 onwards), hence we didn't
see this issue.
Removing statement "D" from "sysv4.h" file so as to generate expected DWARF
register number does work, but will there be any side effects?
Regards,
Rohit
RE: [RFC: Patch, PR 60102] [4.9/4.10 Regression] powerpc fp-bit ices@dwf_regno
> -Original Message- > From: Maciej W. Rozycki [mailto:ma...@codesourcery.com] > To: Ulrich Weigand > Cc: Dharmakan Rohit-B30502; Wienskoski Edmar-RA8797; David Edelsohn; gcc- > patc...@gcc.gnu.org; Alan Modra; Jakub Jelinek > Subject: Re: [RFC: Patch, PR 60102] [4.9/4.10 Regression] powerpc fp-bit > ices@dwf_regno [snip] > > > > Rohit, could you verify whether this fixes the SPE problem? > > Thanks for your work, I have applied your change to my 4.9 setup, rebuilt the > compiler and smoke-tested it with gdb.base/store.exp and my e500v2 multilib > only; powerpc-linux-gnu target. Unfortunately your patch hasn't changed > anything, the same failures remain. > > Just to be sure I haven't missed anything I reverted your change and Rohit's > one as well, rebuilt the compiler and rerun this testing and this time all > failures > were gone. So it looks like your fix didn't completely cover the damage > Rohit's > change caused. :( Ulrich/Maciej, The patch works for me. Tested with GCC v4.9 branch rev 216036 and GCC trunk rev 216027. Regards, Rohit
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@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, rohitarul...@freescale.com
> wrote:
> >> -Original Message-
> >> From: Alan Modra [mailto:amo...@gmail.com]
> >> Sent: Saturday, April 26, 2014 11:52 AM
> >> To: Dharmakan Rohit-B30502
> >> Cc: gcc-patches@gcc.gnu.org; dje@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 +, rohitarul...@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
Ping.
-Original Message-
From: Dharmakan Rohit-B30502
Sent: Friday, March 27, 2015 7:57 PM
To: gcc-patches@gcc.gnu.org; rguent...@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@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, rohitarul...@freescale.com
> wrote:
> >> -Original Message-
> >> From: Alan Modra [mailto:amo...@gmail.com]
> >> Sent: Saturday, April 26, 2014 11:52 AM
> >> To: Dharmakan Rohit-B30502
> >> Cc: gcc-patches@gcc.gnu.org; dje@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 +,
> >> rohitarul...@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
>-Original Message- >From: Jeff Law [mailto:l...@redhat.com] >Sent: Tuesday, April 28, 2015 11:48 PM >To: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org; rguent...@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, rohitarul...@freescale.com wrote: >> Ping. >> >> -Original Message- >> From: Dharmakan Rohit-B30502 >> Sent: Friday, March 27, 2015 7:57 PM >> To: gcc-patches@gcc.gnu.org; rguent...@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
> -Original Message- > From: Jeff Law [mailto:l...@redhat.com] > Sent: Wednesday, April 29, 2015 4:01 AM > To: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org; > rguent...@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, rohitarul...@freescale.com wrote: > > > > > >> -Original Message- > >> From: Jeff Law [mailto:l...@redhat.com] > >> Sent: Tuesday, April 28, 2015 11:48 PM > >> To: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org; > >> rguent...@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, rohitarul...@freescale.com wrote: > >>> Ping. > >>> > >>> -Original Message- > >>> From: Dharmakan Rohit-B30502 > >>> Sent: Friday, March 27, 2015 7:57 PM > >>> To: gcc-patches@gcc.gnu.org; rguent...@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 gcc.fix_pr60158_fixup_table_trunk_fsf Description: gcc.fix_pr60158_fixup_table_trunk_fsf
RE: [RFC: Patch, PR 60158] gcc/varasm.c : Pass actual alignment value to output_constant_pool_2
> -Original Message- > From: Jeff Law [mailto:l...@redhat.com] > Sent: Thursday, April 30, 2015 8:32 PM > To: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org; > rguent...@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, rohitarul...@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
> -Original Message- > From: Jeff Law [mailto:l...@redhat.com] > Sent: Thursday, April 30, 2015 9:14 PM > On 04/30/2015 09:34 AM, rohitarul...@freescale.com wrote: > > > > > >> -Original Message- > >> From: Jeff Law [mailto:l...@redhat.com] > >> Sent: Thursday, April 30, 2015 8:32 PM > >> To: Dharmakan Rohit-B30502; gcc-patches@gcc.gnu.org; > >> rguent...@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, rohitarul...@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
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@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, rohitarul...@freescale.com
> wrote:
> >> -Original Message-
> >> From: Alan Modra [mailto:amo...@gmail.com]
> >> Sent: Saturday, April 26, 2014 11:52 AM
> >> To: Dharmakan Rohit-B30502
> >> Cc: gcc-patches@gcc.gnu.org; dje@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 +,
> >> rohitarul...@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
> >
gcc.fix_pr60158_fixup_table_trunk_fsf_1
Description: gcc.fix_pr60158_fixup_table_trunk_fsf_1
gcc.fix_pr60158_fixup_table_trunk_fsf_2
Description: gcc.fix_pr60158_fixup_table_trunk_fsf_2
RE: [RFC: Patch, PR 60102] [4.9/4.10 Regression] powerpc fp-bit ices@dwf_regno
> From: Ulrich Weigand [mailto:uweig...@de.ibm.com] > Maciej W. Rozycki wrote: > > On Mon, 4 Aug 2014, Edmar wrote: > > > > > Committed on trunk, revision 213596 > > > Committed on 4.9 branch, revision 213597 > > > > This change regressed GDB for e500v2 multilibs, presumably because it > > does not understand the new DWARF register numbers and does not know > > how to map them to hardware registers. > > As I understand it, the change was supposed to only affect GCC internals, all > externally generated debug info was supposed to remain unchanged. > > If there are changes in debug info, something must have gone wrong. Let me check if I can track this down. Regards, Rohit
[Patch, PR 60158] Generate .fixup sections for .data.rel.ro.local entries.
Hello All,
This is related to the following bug:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60158
Test case: Refer below
Target: e500v2
Command line options: -Os -fdata-sections -fpic -mrelocatable -mno-spe
Notes:
GCC v4.7.3 puts the address of "abc" into the GOT directly
GCC v4.8.2 is generating a pointer to the string and putting the
address of that in the GOT. But it doesn't generate the required ".fixup"
table entries.
asm code generated:
...
.section.data.rel.ro.local,"aw",@progbits
.align 2
.LC5:
.4byte .LC0
...
.section.rodata.str1.4,"aMS",@progbits,1
.align 2
.LC0:
.string "abc"
.LC1:
.string "def"
..
1) Compared to GCC v4.7.3, with GCC v4.8 series, there is a new flag
'-fira-hoist-pressure' which is enabled by default at -Os.
Disabling this optimization '-fno-ira-hoist-pressure' generates similar
code as in GCC v4.7.3
2) The actual issue is that with GCC v4.8.2, the move instruction of that
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.
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);
}
else if (TREE_CONSTANT_POOL_ADDRESS_P (symbol))
Source file: gcc-4.8.2/gcc/config/rs6000.c
static bool
rs6000_assemble_integer (rtx x, unsigned int size, int aligned_p)
{
#ifdef RELOCATABLE_NEEDS_FIXUP
/* Special handling for SI values. */
if (RELOCATABLE_NEEDS_FIXUP && size == 4 && aligned_p)
--- (B)
{
Ideally, passing proper alignment to (A) should have worked. But if we pass the
proper alignment to (A) output_constant_pool_1, .align directive gets emitted
twice. Is that the actual purpose of explicitly passing '1' as alignment to
output_constant_pool_1?
- output_constant_pool_1 (desc, 1);
+output_constant_pool_1 (desc, desc->align);
Generated asm with this change:
.section.data.rel.ro.local,"aw",@progbits
.align 2
.align 2
.LC5:
.LCP0:
.long (.LC0)@fixup
Adding a similar change to its helper function "output_constant_pool_2" does
generate the expected code.
[gcc]
2014-04-22 Rohit
PR target/60158
* varasm.c (output_constant_pool_1): Pass actual alignment value to
output_constant_pool_2
to emit ".fixup" section.
[gcc/testsuite]
2014-04-22 Rohit
PR target/60158
* gcc.target/powerpc/pr60158.c: New test. Check if ".fixup" section gets
emitted for
".data.rel.ro.local" section.
Index: gcc/varasm.c
===
--- gcc/varasm.c(revision 209534)
+++ gcc/varasm.c(working copy)
@@ -3771,7 +3771,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. */
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,91 @@
+/* { 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");
+ bre
RE: [Patch, PR 60158] Generate .fixup sections for .data.rel.ro.local entries.
> -Original Message-
> From: Alan Modra [mailto:amo...@gmail.com]
> Sent: Saturday, April 26, 2014 11:52 AM
> To: Dharmakan Rohit-B30502
> Cc: gcc-patches@gcc.gnu.org; dje@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 +, rohitarul...@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
gcc.fix_pr60158_fixup_table-fsf
Description: gcc.fix_pr60158_fixup_table-fsf
gcc.fix_pr60158_fixup_table-fsf-2
Description: gcc.fix_pr60158_fixup_table-fsf-2
[RFC: Patch, PR 60102] [4.9/4.10 Regression] powerpc fp-bit ices at dwf_regno
Hello All, This is related to the following bug: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60102 I have tried to fix the e500v2 build on GCC v4.9.0 with the attached patch. Can you please review and comment on the changes especially DWARF_FRAME_REGNUM, DWARF_REG_TO_UNWIND_COLUMN definitions? Tested this patch on trunk [r212120] with ppc64 and didn't find any new regressions. Back ported this patch on GCC v4.8.2 e500v2 and tested with no new regressions Note: With GCC v4.9.0, to build the e500v2 bareboard version the attached patch is enough. With GCC v4.9.0. to build the e500v2 linux version along with the attached patch please add pr60735 patch too. Regards, Rohit pr60102.patch Description: pr60102.patch
RE: [RFC: Patch, PR 60102] [4.9/4.10 Regression] powerpc fp-bit ices at dwf_regno
Ping! > -Original Message- > From: Dharmakan Rohit-B30502 > Sent: Tuesday, July 08, 2014 8:13 AM > To: gcc-patches@gcc.gnu.org > Cc: Wienskoski Edmar-RA8797; dje@gmail.com; Alan Modra; Jakub Jelinek > Subject: [RFC: Patch, PR 60102] [4.9/4.10 Regression] powerpc fp-bit ices at > dwf_regno > > Hello All, > > This is related to the following bug: > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60102 > > I have tried to fix the e500v2 build on GCC v4.9.0 with the attached patch. > Can you please review and comment on the changes especially > DWARF_FRAME_REGNUM, DWARF_REG_TO_UNWIND_COLUMN definitions? > > Tested this patch on trunk [r212120] with ppc64 and didn't find any new > regressions. > Back ported this patch on GCC v4.8.2 e500v2 and tested with no new > regressions > > Note: > With GCC v4.9.0, to build the e500v2 bareboard version the attached patch is > enough. > With GCC v4.9.0. to build the e500v2 linux version along with the attached > patch please add pr60735 patch too. > > Regards, > Rohit >
RE: [RFC: Patch, PR 60102] [4.9/4.10 Regression] powerpc fp-bit ices@dwf_regno
Ulrich,
Thanks for your comments, I have updated the patch accordingly.
> > /* The SPE has an additional 32 synthetic registers, with DWARF debug
> > info numbering for these registers starting at 1200. While
> > eh_frame @@ -951,13 +952,14 @@ enum data_align { align_abi, align_opt,
> > We must map them here to avoid huge unwinder tables mostly consisting
> > of unused space. */
> > #define DWARF_REG_TO_UNWIND_COLUMN(r) \
> > - ((r) > 1200 ? ((r) - 1200 + (DWARF_FRAME_REGISTERS - 32)) : (r))
> > + ((r) >= FIRST_SPE_HIGH_REGNO ? ((r) - FIRST_SPE_HIGH_REGNO +
> > + (DWARF_FRAME_REGISTERS - 32)) : (r))
>
> As discussed above, this shouldn't change.
Updated to handle first SPE high register too.
#define DWARF_REG_TO_UNWIND_COLUMN(r) \
- ((r) > 1200 ? ((r) - 1200 + (DWARF_FRAME_REGISTERS - 32)) : (r))
+ ((r) >= 1200 ? ((r) - 1200 + (DWARF_FRAME_REGISTERS - 32)) : (r)
Tested this patch on trunk [213030] & GCC v4.9.1 with ppc64 and didn't find any
new regressions.
Back ported this patch on GCC v4.8.3 e500v2 and tested with no new regressions
PR target/60102
[libgcc]
2014-07-31 Rohit
* config/rs6000/linux-unwind.h (ppc_fallback_frame_state): Update
based on change in SPE high register numbers and 3 HTM registers.
[gcc]
2014-07-31 Rohit
* config/rs6000/rs6000.c
(rs6000_reg_names) : Add SPE high register names.
(alt_reg_names) : Likewise
(rs6000_dwarf_register_span) : For SPE high registers, replace
dwarf register numbers with GCC hard register numbers.
(rs6000_init_dwarf_reg_sizes_extra) : Likewise.
(rs6000_dbx_register_number): For SPE high registers, return dwarf
register number for the corresponding GCC hard register number.
* config/rs6000/rs6000.h
(FIRST_PSEUDO_REGISTER) : Update based on 32 newly added GCC hard
register numbers for SPE high registers.
(DWARF_FRAME_REGISTERS) : Likewise.
(DWARF_REG_TO_UNWIND_COLUMN) : Likewise.
(DWARF_FRAME_REGNUM) : Likewise.
(FIXED_REGISTERS) : Likewise.
(CALL_USED_REGISTERS) : Likewise.
(CALL_REALLY_USED_REGISTERS) : Likewise.
(REG_ALLOC_ORDER) : Likewise.
(enum reg_class) : Likewise.
(REG_CLASS_NAMES) : Likewise.
(REG_CLASS_CONTENTS) : Likewise.
(SPE_HIGH_REGNO_P) : New macro to identify SPE high registers.
* gcc.target/powerpc/pr60102.c: New testcase.
Please let me know if you have any further comments on the updated patch.
Regards,
Rohit
pr60102.patch
Description: pr60102.patch
RE: [RFC: Patch, PR 60102] [4.9/4.10 Regression] powerpc fp-bit ices@dwf_regno
Hello Ulrich, Thanks. > > /* Use gcc hard register numbering for eh_frame. */ -#define > >DWARF_FRAME_REGNUM(REGNO) (REGNO) > >+#define DWARF_FRAME_REGNUM(REGNO) \ > >+ ((REGNO) >= FIRST_SPE_HIGH_REGNO ? ((REGNO) - > FIRST_SPE_HIGH_REGNO + > >+1200) : (REGNO)) > > Any reason for not using SPE_HIGH_REGNO_P here, just in case we do get > other hard registers at some point? Yes, we can use it. I just have to move the definition of "SPE_HIGH_REGNO_P" macro before "DWARF_FRAME_REGNUM" macro definition. [Previously, I had defined and placed "SPE_HIGH_REGNO_P" macro along with similar macros "ALTIVEC_REGNO_P" etc.] I had updated the patch as required (For this last change, I have checked/tested only the builds: ppc64 trunk, e500v2 v4.9.1 bareboard & linux build). PR target/60102 [libgcc] 2014-07-31 Rohit * config/rs6000/linux-unwind.h (ppc_fallback_frame_state): Update based on change in SPE high register numbers and 3 HTM registers. [gcc] 2014-07-31 Rohit * config/rs6000/rs6000.c (rs6000_reg_names) : Add SPE high register names. (alt_reg_names) : Likewise. (rs6000_dwarf_register_span) : For SPE high registers, replace dwarf register numbers with GCC hard register numbers. (rs6000_init_dwarf_reg_sizes_extra) : Likewise. (rs6000_dbx_register_number): For SPE high registers, return dwarf register number for the corresponding GCC hard register number. * config/rs6000/rs6000.h (FIRST_PSEUDO_REGISTER) : Update based on 32 newly added GCC hard register numbers for SPE high registers. (DWARF_FRAME_REGISTERS) : Likewise. (DWARF_REG_TO_UNWIND_COLUMN) : Likewise. (DWARF_FRAME_REGNUM) : Likewise. (FIXED_REGISTERS) : Likewise. (CALL_USED_REGISTERS) : Likewise. (CALL_REALLY_USED_REGISTERS) : Likewise. (REG_ALLOC_ORDER) : Likewise. (enum reg_class) : Likewise. (REG_CLASS_NAMES) : Likewise. (REG_CLASS_CONTENTS) : Likewise. (SPE_HIGH_REGNO_P) : New macro to identify SPE high registers. * gcc.target/powerpc/pr60102.c: New testcase. Regards, Rohit pr60102.patch Description: pr60102.patch
RE: [RFC: Patch, PR 60102] [4.9/4.10 Regression] powerpc fp-bit ices@dwf_regno
Jakub,
> On Fri, Aug 01, 2014 at 06:03:56PM +0000, rohitarul...@freescale.com wrote:
> > PR target/60102
>
> --- libgcc/config/rs6000/linux-unwind.h (revision 213110)
> +++ libgcc/config/rs6000/linux-unwind.h (working copy)
> @@ -274,8 +274,8 @@ ppc_fallback_frame_state (struct _Unwind
> #ifdef __SPE__
>for (i = 14; i < 32; i++)
> {
> - fs->regs.reg[i + FIRST_PSEUDO_REGISTER - 1].how = REG_SAVED_OFFSET;
> - fs->regs.reg[i + FIRST_PSEUDO_REGISTER - 1].loc.offset
> + fs->regs.reg[i + FIRST_SPE_HIGH_REGNO - 4].how = REG_SAVED_OFFSET;
> + fs->regs.reg[i + FIRST_SPE_HIGH_REGNO - 4].loc.offset
> = (long) ®s->vregs - new_cfa + 4 * i;
> }
> #endif
>
> is a different index, previously i + 116, newly i + 113, is that intentional?
>
Yes, it is intentional.
This part of code wasn't updated after the introduction of three HTM registers.
Regards,
Rohit
RE: [RFC: Patch, PR 60102] [4.9/4.10 Regression] powerpc fp-bit ices@dwf_regno
Jakub, > On Mon, Aug 04, 2014 at 11:51:34AM -0500, Edmar wrote: > > Committed on trunk, revision 213596 > > Committed on 4.9 branch, revision 213597 > > Note the ChangeLog entry was grossly misformated. > I've fixed it up in gcc/ChangeLog on the trunk, but not on the branch > nor in libgcc. There should be no space before :, all lines > in ChangeLog entry should be just tab indented rather than tab + 2 spaces, > and filenames, unless they are too long, shouldn't be alone on the lines. > And testsuite entries shouldn't go into gcc/ChangeLog. Sorry about that. I have updated the ChangeLog entries accordingly. Edmar, can you please commit these patches? Trunk: pr60102-ChangeLog-trunk.patch GCC v4.9 branch: pr60102-ChangeLog-v49.patch Regards, Rohit pr60102-ChangeLog-trunk.patch Description: pr60102-ChangeLog-trunk.patch pr60102-ChangeLog-v49.patch Description: pr60102-ChangeLog-v49.patch
