On Tue, 2013-10-01 at 12:19 +0200, Richard Biener wrote: > On Wed, Sep 25, 2013 at 1:37 PM, Yufeng Zhang <yufeng.zh...@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? 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.zh...@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. >
