RE: Shouldn't convert_scalars_to_vector call free_dominance_info?

["Enkovich, Ilya" <ilya dot enkovich at intel dot com>]

> On Thu, Mar 10, 2016 at 5:41 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> > On Thu, Mar 10, 2016 at 5:24 AM, Richard Biener
> > <richard.guenther@gmail.com> wrote:
> >> On Thu, Mar 10, 2016 at 2:16 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> >>> On Thu, Mar 10, 2016 at 5:01 AM, Richard Biener
> >>> <richard.guenther@gmail.com> wrote:
> >>>> On Thu, Mar 10, 2016 at 1:48 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> >>>>> convert_scalars_to_vector in i386.c calls
> >>>>>
> >>>>> calculate_dominance_info (CDI_DOMINATORS);
> >>>>>
> >>>>> Shouldn't it call
> >>>>>
> >>>>> free_dominance_info (CDI_DOMINATORS);
> >>>>>
> >>>>> after it is done like other places where calculate_dominance_info is
> used?
> >>>>
> >>>> Only if it invalidates it.   Other than post-dominator info dominator
> >>>> info is kept
> >>>> up-to-date between passes.
> >>>>
> >>>>> When I extend the STV pass to 64-bit and put the 64-bit STV pass
> >>>>> before the CSE pass, I run into
> >>>>>
> >>>>> gcc_assert (!dom_info_available_p (CDI_DOMINATORS));
> >>>>
> >>>> That looks like a bogus assert to me.  Which one is it?  The one in
> >>>> cfgcleanup.c?
> >>>> In that case something else should have freed it (the thing that
> >>>> doesn't preserve it) or the condition the assert is guarded is
> >>>> "incorrect".
> >>>
> >>> cleanup_cfg has
> >>>
> >>>  /* ???  We probably do this way too often.  */
> >>>   if (current_loops
> >>>       && (changed
> >>>           || (mode & CLEANUP_CFG_CHANGED)))
> >>>     {
> >>>       timevar_push (TV_REPAIR_LOOPS);
> >>>       /* The above doesn't preserve dominance info if available.  */
> >>>       gcc_assert (!dom_info_available_p (CDI_DOMINATORS));
> >>>       calculate_dominance_info (CDI_DOMINATORS);
> >>>       fix_loop_structure (NULL);
> >>>       free_dominance_info (CDI_DOMINATORS);
> >>>       timevar_pop (TV_REPAIR_LOOPS);
> >>>     }
> >>>
> >>> Should assert be removed?
> >>
> >> I think these days we can safely remove it as
> >> calculate_dominance_info will ICE if dom info is said to be present
> >> but is incorrect.  I put in the assert to catch this case before we
> >> had that.
> >
> > Remove that assert doesn't solve all problems.  Now I got
> >
> > 0x98483a verify_dominators(cdi_direction)
> >         /export/gnu/import/git/sources/gcc/gcc/dominance.c:1038
> > 0x982591 checking_verify_dominators
> >         /export/gnu/import/git/sources/gcc/gcc/dominance.h:71
> > 0x983d27 calculate_dominance_info(cdi_direction)
> >         /export/gnu/import/git/sources/gcc/gcc/dominance.c:664
> > 0x1742a20 fwprop_init
> >         /export/gnu/import/git/sources/gcc/gcc/fwprop.c:1393
> > 0x1742cf8 fwprop_addr
> >         /export/gnu/import/git/sources/gcc/gcc/fwprop.c:1511
> > 0x1742e60 execute
> >         /export/gnu/import/git/sources/gcc/gcc/fwprop.c:1557
> > Please submit a full bug report,
> > with preprocessed source if appropriate.
> > Please include the complete backtrace with any bug report.
> > See <http://gcc.gnu.org/bugs.html> for instructions.
> > Makefile:612: recipe for target 'bid128_fma.o' failed
> > make[2]: *** [bid128_fma.o] Error 1
> >
> > if convert_scalars_to_vector doesn't call
> >
> > free_dominance_info (CDI_DOMINATORS);
> 
> Since convert_scalars_to_vector may add instructions, dominance info is no
> longer up to date.

The only CFG change STV makes is via split_edge.  But split_edge fixes up dominance info.  I don't see how it may become invalid.

Thanks,
Ilya

> 
> --
> H.J.