| Message ID | CAAe5K+UcnQhUZ6WdOLdMSABa+9FTQLE+jiUJ_QrZ2BwvhAf_gA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 01/15/14 10:07, Teresa Johnson wrote: > Handle NULL vdef for call in the case where we have a matching vnresult > that has a vdef (it already handles the NULL vdef case when !vnresult). This > can happen for promoted indirect calls if the fallback indirect call > (which has a vdef) can be proven equivalent to the promoted direct call > (which might not have a vdef). > > This occurred for a case where we had a promoted indirect call, > where FRE determined that the promoted direct call and the fall-back indirect > call were equivalent (since earlier it determined that the function pointer > was always set to that target). The indirect call had been analyzed by > visit_reference_op_call first, and had a VDEF. The direct call did not have a > VDEF, presumably because it was a leaf function in the same module without any > stores. But visit_reference_op_call unconditionally calls set_ssa_val_to when > the previous vnresult had a vdef, leading to a seg fault in this case. > If we had analyzed the direct call first the failure wouldn't have occurred > since the !vnresult case guards the call to set_ssa_val_to with a check > for a NULL vdef, and the subsequent handling of the indirect call would > also not call set_ssa_val_to since vnresult would have had a null result_vdef. > > Bootstrapped and tested on x86_64-unknown-linux-gnu. Ok for trunk? > > 2014-01-15 Teresa Johnson <tejohnson@google.com> > > * tree-ssa-sccvn.c (visit_reference_op_call): Handle NULL vdef. The patch is OK. Given this was an ICE, do you have a reduced test we can add to the regression suite? I realize that order of visiting in the SCC is important to trigger, but a regression test would still be useful. Thanks, Jeff
On Wed, Jan 15, 2014 at 10:46 AM, Jeff Law <law@redhat.com> wrote: > On 01/15/14 10:07, Teresa Johnson wrote: >> >> Handle NULL vdef for call in the case where we have a matching vnresult >> that has a vdef (it already handles the NULL vdef case when !vnresult). >> This >> can happen for promoted indirect calls if the fallback indirect call >> (which has a vdef) can be proven equivalent to the promoted direct call >> (which might not have a vdef). >> >> This occurred for a case where we had a promoted indirect call, >> where FRE determined that the promoted direct call and the fall-back >> indirect >> call were equivalent (since earlier it determined that the function >> pointer >> was always set to that target). The indirect call had been analyzed by >> visit_reference_op_call first, and had a VDEF. The direct call did not >> have a >> VDEF, presumably because it was a leaf function in the same module without >> any >> stores. But visit_reference_op_call unconditionally calls set_ssa_val_to >> when >> the previous vnresult had a vdef, leading to a seg fault in this case. >> If we had analyzed the direct call first the failure wouldn't have >> occurred >> since the !vnresult case guards the call to set_ssa_val_to with a check >> for a NULL vdef, and the subsequent handling of the indirect call would >> also not call set_ssa_val_to since vnresult would have had a null >> result_vdef. >> >> Bootstrapped and tested on x86_64-unknown-linux-gnu. Ok for trunk? >> >> 2014-01-15 Teresa Johnson <tejohnson@google.com> >> >> * tree-ssa-sccvn.c (visit_reference_op_call): Handle NULL vdef. > > The patch is OK. Given this was an ICE, do you have a reduced test we can > add to the regression suite? I realize that order of visiting in the SCC is > important to trigger, but a regression test would still be useful. Unfortunately it was hit using LIPO on the google/4_8 branch, and only occurred with a specific profile. That's why I don't have a trunk test case. I suppose I could create a test case that has a similar opportunity. It does look like there are some indirect call promotion with FDO tests already (e.g. gcc.dg/tree-prof/indir-call-prof.c), but I'm not sure whether they even trigger the same type of FRE opportunity. I will take a look. Teresa > > Thanks, > Jeff > >
On Wed, Jan 15, 2014 at 12:00 PM, Teresa Johnson <tejohnson@google.com> wrote: > On Wed, Jan 15, 2014 at 10:46 AM, Jeff Law <law@redhat.com> wrote: >> On 01/15/14 10:07, Teresa Johnson wrote: >>> >>> Handle NULL vdef for call in the case where we have a matching vnresult >>> that has a vdef (it already handles the NULL vdef case when !vnresult). >>> This >>> can happen for promoted indirect calls if the fallback indirect call >>> (which has a vdef) can be proven equivalent to the promoted direct call >>> (which might not have a vdef). >>> >>> This occurred for a case where we had a promoted indirect call, >>> where FRE determined that the promoted direct call and the fall-back >>> indirect >>> call were equivalent (since earlier it determined that the function >>> pointer >>> was always set to that target). The indirect call had been analyzed by >>> visit_reference_op_call first, and had a VDEF. The direct call did not >>> have a >>> VDEF, presumably because it was a leaf function in the same module without >>> any >>> stores. But visit_reference_op_call unconditionally calls set_ssa_val_to >>> when >>> the previous vnresult had a vdef, leading to a seg fault in this case. >>> If we had analyzed the direct call first the failure wouldn't have >>> occurred >>> since the !vnresult case guards the call to set_ssa_val_to with a check >>> for a NULL vdef, and the subsequent handling of the indirect call would >>> also not call set_ssa_val_to since vnresult would have had a null >>> result_vdef. >>> >>> Bootstrapped and tested on x86_64-unknown-linux-gnu. Ok for trunk? >>> >>> 2014-01-15 Teresa Johnson <tejohnson@google.com> >>> >>> * tree-ssa-sccvn.c (visit_reference_op_call): Handle NULL vdef. >> >> The patch is OK. Given this was an ICE, do you have a reduced test we can >> add to the regression suite? I realize that order of visiting in the SCC is >> important to trigger, but a regression test would still be useful. > > Unfortunately it was hit using LIPO on the google/4_8 branch, and only > occurred with a specific profile. That's why I don't have a trunk test > case. I suppose I could create a test case that has a similar > opportunity. It does look like there are some indirect call promotion > with FDO tests already (e.g. gcc.dg/tree-prof/indir-call-prof.c), but > I'm not sure whether they even trigger the same type of FRE > opportunity. I will take a look. I'm having a hard time getting the right combination of early/late inlining and indirect call promotion on trunk to occur to even allow this optimization to kick in. It's possible I could do so with a sufficiently complicated test, but I'm not sure it is worth it. I'll commit the fix right now though. Thanks, Teresa > > Teresa > >> >> Thanks, >> Jeff >> >> > > > > -- > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
There are options you can use to control passes explicitly: -fdisable-... -fenable-.... To disable early inline: -fdisable-tree-einline David On Wed, Jan 15, 2014 at 1:17 PM, Teresa Johnson <tejohnson@google.com> wrote: > On Wed, Jan 15, 2014 at 12:00 PM, Teresa Johnson <tejohnson@google.com> wrote: >> On Wed, Jan 15, 2014 at 10:46 AM, Jeff Law <law@redhat.com> wrote: >>> On 01/15/14 10:07, Teresa Johnson wrote: >>>> >>>> Handle NULL vdef for call in the case where we have a matching vnresult >>>> that has a vdef (it already handles the NULL vdef case when !vnresult). >>>> This >>>> can happen for promoted indirect calls if the fallback indirect call >>>> (which has a vdef) can be proven equivalent to the promoted direct call >>>> (which might not have a vdef). >>>> >>>> This occurred for a case where we had a promoted indirect call, >>>> where FRE determined that the promoted direct call and the fall-back >>>> indirect >>>> call were equivalent (since earlier it determined that the function >>>> pointer >>>> was always set to that target). The indirect call had been analyzed by >>>> visit_reference_op_call first, and had a VDEF. The direct call did not >>>> have a >>>> VDEF, presumably because it was a leaf function in the same module without >>>> any >>>> stores. But visit_reference_op_call unconditionally calls set_ssa_val_to >>>> when >>>> the previous vnresult had a vdef, leading to a seg fault in this case. >>>> If we had analyzed the direct call first the failure wouldn't have >>>> occurred >>>> since the !vnresult case guards the call to set_ssa_val_to with a check >>>> for a NULL vdef, and the subsequent handling of the indirect call would >>>> also not call set_ssa_val_to since vnresult would have had a null >>>> result_vdef. >>>> >>>> Bootstrapped and tested on x86_64-unknown-linux-gnu. Ok for trunk? >>>> >>>> 2014-01-15 Teresa Johnson <tejohnson@google.com> >>>> >>>> * tree-ssa-sccvn.c (visit_reference_op_call): Handle NULL vdef. >>> >>> The patch is OK. Given this was an ICE, do you have a reduced test we can >>> add to the regression suite? I realize that order of visiting in the SCC is >>> important to trigger, but a regression test would still be useful. >> >> Unfortunately it was hit using LIPO on the google/4_8 branch, and only >> occurred with a specific profile. That's why I don't have a trunk test >> case. I suppose I could create a test case that has a similar >> opportunity. It does look like there are some indirect call promotion >> with FDO tests already (e.g. gcc.dg/tree-prof/indir-call-prof.c), but >> I'm not sure whether they even trigger the same type of FRE >> opportunity. I will take a look. > > I'm having a hard time getting the right combination of early/late > inlining and indirect call promotion on trunk to occur to even allow > this optimization to kick in. It's possible I could do so with a > sufficiently complicated test, but I'm not sure it is worth it. I'll > commit the fix right now though. > > Thanks, > Teresa > >> >> Teresa >> >>> >>> Thanks, >>> Jeff >>> >>> >> >> >> >> -- >> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 > > > > -- > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
I did try disabling early inlining and some other things but still didn't have luck. The indirect call promotion is also in a different phase in trunk and google/4_8, which was adding another complication. Teresa On Wed, Jan 15, 2014 at 1:23 PM, Xinliang David Li <davidxl@google.com> wrote: > There are options you can use to control passes explicitly: > -fdisable-... -fenable-.... > > To disable early inline: > > -fdisable-tree-einline > > David > > On Wed, Jan 15, 2014 at 1:17 PM, Teresa Johnson <tejohnson@google.com> wrote: >> On Wed, Jan 15, 2014 at 12:00 PM, Teresa Johnson <tejohnson@google.com> wrote: >>> On Wed, Jan 15, 2014 at 10:46 AM, Jeff Law <law@redhat.com> wrote: >>>> On 01/15/14 10:07, Teresa Johnson wrote: >>>>> >>>>> Handle NULL vdef for call in the case where we have a matching vnresult >>>>> that has a vdef (it already handles the NULL vdef case when !vnresult). >>>>> This >>>>> can happen for promoted indirect calls if the fallback indirect call >>>>> (which has a vdef) can be proven equivalent to the promoted direct call >>>>> (which might not have a vdef). >>>>> >>>>> This occurred for a case where we had a promoted indirect call, >>>>> where FRE determined that the promoted direct call and the fall-back >>>>> indirect >>>>> call were equivalent (since earlier it determined that the function >>>>> pointer >>>>> was always set to that target). The indirect call had been analyzed by >>>>> visit_reference_op_call first, and had a VDEF. The direct call did not >>>>> have a >>>>> VDEF, presumably because it was a leaf function in the same module without >>>>> any >>>>> stores. But visit_reference_op_call unconditionally calls set_ssa_val_to >>>>> when >>>>> the previous vnresult had a vdef, leading to a seg fault in this case. >>>>> If we had analyzed the direct call first the failure wouldn't have >>>>> occurred >>>>> since the !vnresult case guards the call to set_ssa_val_to with a check >>>>> for a NULL vdef, and the subsequent handling of the indirect call would >>>>> also not call set_ssa_val_to since vnresult would have had a null >>>>> result_vdef. >>>>> >>>>> Bootstrapped and tested on x86_64-unknown-linux-gnu. Ok for trunk? >>>>> >>>>> 2014-01-15 Teresa Johnson <tejohnson@google.com> >>>>> >>>>> * tree-ssa-sccvn.c (visit_reference_op_call): Handle NULL vdef. >>>> >>>> The patch is OK. Given this was an ICE, do you have a reduced test we can >>>> add to the regression suite? I realize that order of visiting in the SCC is >>>> important to trigger, but a regression test would still be useful. >>> >>> Unfortunately it was hit using LIPO on the google/4_8 branch, and only >>> occurred with a specific profile. That's why I don't have a trunk test >>> case. I suppose I could create a test case that has a similar >>> opportunity. It does look like there are some indirect call promotion >>> with FDO tests already (e.g. gcc.dg/tree-prof/indir-call-prof.c), but >>> I'm not sure whether they even trigger the same type of FRE >>> opportunity. I will take a look. >> >> I'm having a hard time getting the right combination of early/late >> inlining and indirect call promotion on trunk to occur to even allow >> this optimization to kick in. It's possible I could do so with a >> sufficiently complicated test, but I'm not sure it is worth it. I'll >> commit the fix right now though. >> >> Thanks, >> Teresa >> >>> >>> Teresa >>> >>>> >>>> Thanks, >>>> Jeff >>>> >>>> >>> >>> >>> >>> -- >>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >> >> >> >> -- >> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
Index: tree-ssa-sccvn.c =================================================================== --- tree-ssa-sccvn.c (revision 206100) +++ tree-ssa-sccvn.c (working copy) @@ -2792,7 +2792,7 @@ visit_reference_op_call (tree lhs, gimple stmt) if (vnresult) { - if (vnresult->result_vdef) + if (vnresult->result_vdef && vdef) changed |= set_ssa_val_to (vdef, vnresult->result_vdef); if (!vnresult->result && lhs)
Handle NULL vdef for call in the case where we have a matching vnresult that has a vdef (it already handles the NULL vdef case when !vnresult). This can happen for promoted indirect calls if the fallback indirect call (which has a vdef) can be proven equivalent to the promoted direct call (which might not have a vdef). This occurred for a case where we had a promoted indirect call, where FRE determined that the promoted direct call and the fall-back indirect call were equivalent (since earlier it determined that the function pointer was always set to that target). The indirect call had been analyzed by visit_reference_op_call first, and had a VDEF. The direct call did not have a VDEF, presumably because it was a leaf function in the same module without any stores. But visit_reference_op_call unconditionally calls set_ssa_val_to when the previous vnresult had a vdef, leading to a seg fault in this case. If we had analyzed the direct call first the failure wouldn't have occurred since the !vnresult case guards the call to set_ssa_val_to with a check for a NULL vdef, and the subsequent handling of the indirect call would also not call set_ssa_val_to since vnresult would have had a null result_vdef. Bootstrapped and tested on x86_64-unknown-linux-gnu. Ok for trunk? 2014-01-15 Teresa Johnson <tejohnson@google.com> * tree-ssa-sccvn.c (visit_reference_op_call): Handle NULL vdef.