| Message ID | CAO2gOZUEuZmWSMSP2fVo=caQgLbJ3c+kCw-5ya6UKEuyYVKOfw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
> I looked at this problem. Bug updated > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 > > This is a bug when updating block during tree-inline. Basically, it is > legal for *n to be NULL. E.g. When gimple_block(id->gimple_call) is > NULL, remap_blocks_to_null will be called to set *n to NULL. The NULL in gimple_block (gimple_call) comes from the call introduced by ipa-split? I remember that ipa-split used to try to put the call into block since we was ICEing in similar ways previously, too. Perhaps this has changed with new BLOCK representation? Honza
On Fri, Oct 4, 2013 at 11:54 AM, Jan Hubicka <hubicka@ucw.cz> wrote: >> I looked at this problem. Bug updated >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 >> >> This is a bug when updating block during tree-inline. Basically, it is >> legal for *n to be NULL. E.g. When gimple_block(id->gimple_call) is >> NULL, remap_blocks_to_null will be called to set *n to NULL. > > The NULL in gimple_block (gimple_call) comes from the call introduced by ipa-split? That is correct. > I remember that ipa-split used to try to put the call into block since we was ICEing > in similar ways previously, too. Perhaps this has changed with new BLOCK representation? The new BLOCK representation does not change this. I think it makes sense to leave the block of newly introduced call_stmt as NULL because when it's inlined back, we don't want to add additional block layers. Dehao > > Honza
> On Fri, Oct 4, 2013 at 11:54 AM, Jan Hubicka <hubicka@ucw.cz> wrote: > >> I looked at this problem. Bug updated > >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 > >> > >> This is a bug when updating block during tree-inline. Basically, it is > >> legal for *n to be NULL. E.g. When gimple_block(id->gimple_call) is > >> NULL, remap_blocks_to_null will be called to set *n to NULL. > > > > The NULL in gimple_block (gimple_call) comes from the call introduced by ipa-split? > > That is correct. > > > I remember that ipa-split used to try to put the call into block since we was ICEing > > in similar ways previously, too. Perhaps this has changed with new BLOCK representation? > > The new BLOCK representation does not change this. I think it makes > sense to leave the block of newly introduced call_stmt as NULL because > when it's inlined back, we don't want to add additional block layers. You are right, it may be result of Jakub's changes in the area (to improve debug info after inlining back). I guess the patch makes sense then. Honza > > Dehao > > > > > Honza
On 10/04/13 22:23, Jan Hubicka wrote: >> On Fri, Oct 4, 2013 at 11:54 AM, Jan Hubicka <hubicka@ucw.cz> wrote: >>>> I looked at this problem. Bug updated >>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 >>>> >>>> This is a bug when updating block during tree-inline. Basically, it is >>>> legal for *n to be NULL. E.g. When gimple_block(id->gimple_call) is >>>> NULL, remap_blocks_to_null will be called to set *n to NULL. >>> >>> The NULL in gimple_block (gimple_call) comes from the call introduced by ipa-split? >> >> That is correct. >> >>> I remember that ipa-split used to try to put the call into block since we was ICEing >>> in similar ways previously, too. Perhaps this has changed with new BLOCK representation? >> >> The new BLOCK representation does not change this. I think it makes >> sense to leave the block of newly introduced call_stmt as NULL because >> when it's inlined back, we don't want to add additional block layers. > > You are right, it may be result of Jakub's changes in the area (to improve debug info > after inlining back). I guess the patch makes sense then. It at-least fixes the issues I've been seeing on arm-none-linux-gnueabihf. The build has atleast gone past that point and I should have some test results later today. I don't know enough in that area to comment further on the technical aspects of the patch but it doesn't look outrageous from my point of view . Can someone comment / approve it quickly so that we get AArch32 and AArch64 linux cross-builds back up ? regards Ramana > > Honza > >> >> Dehao >> >>> >>> Honza >
On Mon, Oct 7, 2013 at 12:02 PM, Ramana Radhakrishnan <ramrad01@arm.com> wrote: > On 10/04/13 22:23, Jan Hubicka wrote: >>> >>> On Fri, Oct 4, 2013 at 11:54 AM, Jan Hubicka <hubicka@ucw.cz> wrote: >>>>> >>>>> I looked at this problem. Bug updated >>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 >>>>> >>>>> This is a bug when updating block during tree-inline. Basically, it is >>>>> legal for *n to be NULL. E.g. When gimple_block(id->gimple_call) is >>>>> NULL, remap_blocks_to_null will be called to set *n to NULL. >>>> >>>> >>>> The NULL in gimple_block (gimple_call) comes from the call introduced by >>>> ipa-split? >>> >>> >>> That is correct. >>> >>>> I remember that ipa-split used to try to put the call into block since >>>> we was ICEing >>>> in similar ways previously, too. Perhaps this has changed with new >>>> BLOCK representation? >>> >>> >>> The new BLOCK representation does not change this. I think it makes >>> sense to leave the block of newly introduced call_stmt as NULL because >>> when it's inlined back, we don't want to add additional block layers. >> >> >> You are right, it may be result of Jakub's changes in the area (to improve >> debug info >> after inlining back). I guess the patch makes sense then. > > > It at-least fixes the issues I've been seeing on arm-none-linux-gnueabihf. > The build has atleast gone past that point and I should have some test > results later today. > > I don't know enough in that area to comment further on the technical aspects > of the patch but it doesn't look outrageous from my point of view . > > Can someone comment / approve it quickly so that we get AArch32 and AArch64 > linux cross-builds back up ? Ok. Thanks, Richard. > > regards > Ramana > >> >> Honza >> >>> >>> Dehao >>> >>>> >>>> Honza >> >> > >
>> Can someone comment / approve it quickly so that we get AArch32 and AArch64 >> linux cross-builds back up ? > > Ok. Applied for Dehao as r203269 . Tests on arm came back ok. Ramana > > Thanks, > Richard. > >> >> regards >> Ramana >> >>> >>> Honza >>> >>>> >>>> Dehao >>>> >>>>> >>>>> Honza >>> >>> >> >>
Thanks for applying the patch. Backported to google-4_8 I still have some concern when inlining .part function into its original function: basically, the gimple_block for that call may be NULL, but it does not make sense to clear all block info for all stmts in the .part function. Dehao On Tue, Oct 8, 2013 at 1:35 AM, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: >>> Can someone comment / approve it quickly so that we get AArch32 and AArch64 >>> linux cross-builds back up ? >> >> Ok. > > Applied for Dehao as r203269 . Tests on arm came back ok. > > Ramana > >> >> Thanks, >> Richard. >> >>> >>> regards >>> Ramana >>> >>>> >>>> Honza >>>> >>>>> >>>>> Dehao >>>>> >>>>>> >>>>>> Honza >>>> >>>> >>> >>>
Index: gcc/tree-inline.c =================================================================== --- gcc/tree-inline.c (revision 203208) +++ gcc/tree-inline.c (working copy) @@ -2090,7 +2090,10 @@ copy_phis_for_bb (basic_block bb, copy_body_data * n = (tree *) pointer_map_contains (id->decl_map, LOCATION_BLOCK (locus)); gcc_assert (n); - locus = COMBINE_LOCATION_DATA (line_table, locus, *n); + if (*n) + locus = COMBINE_LOCATION_DATA (line_table, locus, *n); + else + locus = LOCATION_LOCUS (locus); } else locus = LOCATION_LOCUS (locus);