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.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.
Sorry, that was a pre-coffee comment. I would like also to see a test that verifies the expected gimple, though, not just that the test runs. > > 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. > >
