Re: [PATCH GCC] Tweak gimple-ssa-strength-reduction.c:backtrace_base_for_ref () to cover different cases as seen on AArch64

[Bill Schmidt <wschmidt at linux dot vnet dot ibm dot com>]

On Tue, 2013-10-01 at 08:17 -0500, Bill Schmidt wrote:
> On Tue, 2013-10-01 at 12:19 +0200, Richard Biener wrote:
> > On Wed, Sep 25, 2013 at 1:37 PM, Yufeng Zhang <Yufeng.Zhang@arm.com> wrote:
> > > Hello,
> > >
> > > Please find the updated version of the patch in the attachment.  It has
> > > addressed the previous comments and also included some changes in order to
> > > pass the bootstrapping on x86_64.
> > >
> > > It's also passed the regtest on arm-none-eabi and aarch64-none-elf.
> > >
> > > It will also fix the test failure as reported here:
> > > http://gcc.gnu.org/ml/gcc-patches/2013-09/msg01317.html
> > >
> > > OK for the trunk?
> > 
> > +   where n is a 32-bit unsigned int and pointer are 64-bit long.  In this
> > +   case, the gimple for (n - 1) is:
> > +
> > +     _2 = n_1(D) + 4294967295; // 0xFFFFFFFF
> > +
> > +   and it is wrong to multiply the large constant by 4 in the 64-bit space.  */
> > +
> > +static bool
> > +safe_to_multiply_p (tree type, double_int cst)
> > +{
> > +  if (TYPE_UNSIGNED (type)
> > +      && ! double_int_fits_to_tree_p (signed_type_for (type), cst))
> > +    return false;
> > +
> > +  return true;
> > +}
> > 
> > This looks wrong.  The only relevant check is as whether the
> > multiplication overflows the original type as you miss the implicit
> > truncation that happens.  Which is something you don't know
> > unless you know the value.  It definitely isn't a property of a type
> > and a constant but the property of two constants and a type.
> > Or the predicate has a wrong name.
> > 
> > The use of get_unwidened in this core routine looks like this is
> > all happening in the wrong place and we should have picked up
> > another candidate for this instead?  I'm sure Bill will know more here.
> 
> I'm not happy with how this patch is progressing.  Without having looked
> too deeply, this might be better handled earlier when determining which
> casts are safe to use in building candidates.  What you have here seems
> more like closing the barn door after the horse got out.  Maybe that's
> the only solution, but it doesn't seem likely.
> 
> Another problem is that your test case isn't testing anything except
> that the compiler doesn't crash.  That isn't sufficient as a regression
> test.
> 
> I'll spend some time looking at this to see if I can find a better
> approach.  It might be a day or two before I can get to it.  In addition
> to the included test case, are there any other cases you've found that I
> should be concerned with?

To help me investigate this without having to build a cross compiler,
could you please compile your test case (without the patch applied)
using -fdump-tree-reassoc2 -fdump-tree-slsr-details and send me the
generated dump files?

Thanks,
Bill

> 
> Thanks,
> Bill
> 
> > 
> > Richard.
> > 
> > 
> > 
> > > Thanks,
> > > Yufeng
> > >
> > >
> > > gcc/
> > >
> > >         * gimple-ssa-strength-reduction.c (safe_to_multiply_p): New
> > > function.
> > >         (backtrace_base_for_ref): Call get_unwidened, check 'base_in'
> > >         again and set unwidend_p with true; call safe_to_multiply_p to avoid
> > >         unsafe unwidened cases.
> > >
> > > gcc/testsuite/
> > >
> > >         * gcc.dg/tree-ssa/slsr-40.c: New test.
> > >
> > >
> > >
> > >
> > > On 09/11/13 13:39, Bill Schmidt wrote:
> > >>
> > >> On Wed, 2013-09-11 at 10:32 +0200, Richard Biener wrote:
> > >>>
> > >>> On Tue, Sep 10, 2013 at 5:53 PM, Yufeng Zhang<Yufeng.Zhang@arm.com>
> > >>> wrote:
> > >>>>
> > >>>> Hi,
> > >>>>
> > >>>> Following Bin's patch in
> > >>>> http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00695.html, this patch
> > >>>> tweaks
> > >>>> backtrace_base_for_ref () to strip of any widening conversion after the
> > >>>> first TREE_CODE check fails.  Without this patch, the test
> > >>>> (gcc.dg/tree-ssa/slsr-39.c) in Bin's patch will fail on AArch64, as
> > >>>> backtrace_base_for_ref () will stop if not seeing an ssa_name since the
> > >>>> tree
> > >>>> code can be nop_expr instead.
> > >>>>
> > >>>> Regtested on arm and aarch64; still bootstrapping x86_64.
> > >>>>
> > >>>> OK for the trunk if the x86_64 bootstrap succeeds?
> > >>>
> > >>>
> > >>> Please add a testcase.
> > >>
> > >>
> > >> Also, the comment "Strip of" should read "Strip off".  Otherwise I have
> > >> no comments.
> > >>
> > >> Thanks,
> > >> Bill
> > >>
> > >>>
> > >>> Richard.
> >