This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



On Tue, 2013-10-01 at 11:56 -0500, Bill Schmidt wrote:
> OK, thanks.  The problem that you've encountered is that you are
> attempting to do something illegal. ;)  (Bin's original patch is
> actually to blame for that, as well as me for not catching it then.)
> 
> As your new test shows, it is unsafe to do the transformation in
> backtrace_base_for_ref when widening from an unsigned type, because the
> unsigned type has wrap semantics by default.  (The actual test must be
> done on TYPE_OVERFLOW_WRAPS since this wrap semantics can be added or
> removed by compile option -- see the comments with legal_cast_p and
> legal_cast_p_1 later in the module.)
> 
> You cannot in general prove that the transformation is allowable for a
> specific constant, because you don't know that what you're adding it to
> won't cause an overflow that's handled incorrectly.
> 
> I believe the correct fix for the unsigned-overflow case is to fail
> backtrace_base_for_ref if legal_cast_p (in_type, out_type) returns
> false, where in_type is the type of the new *PBASE, and out_type is the
> widening type that you're looking through.  So you can't just
> STRIP_NOPS, you have to check the cast for legitimacy for this
> transformation.
> 
> This does not explain why backtrace_base_for_ref does not find all the
> opportunities on slsr-39.c.  I don't immediately see what's preventing
> that.  Note that the transformation is legal in that case because you
> are widening from a signed int to an unsigned int, which won't cause
> problems.  You guys need to dig deeper into why those opportunities are
> missed when sizetype is larger than int.  Let me know if you need help
> figuring it out.

Sorry, I had to leave before and wanted to get this response back to you
in case I didn't get back soon.  I've looked at this some more, and your
general approach should work ok once you get the legal_cast_p check in
place where you do the get_unwidened call now.  Once you know you have a
legal widening, you don't have to worry about the safe_to_multiply_p
stuff.  I.e., you don't need the last two chunks in the patch to
backtrace_base_for_ref, and you don't need the unwidened_p variable.  It
should all fall out properly by just restricting your unwidening to
legal casts.

Thanks,
Bill

> 
> Thanks,
> Bill
> 
> On Tue, 2013-10-01 at 16:06 +0100, Yufeng Zhang wrote:
> > Hi Bill,
> > 
> > Thank you for the review and the offer to help.
> > 
> > On 10/01/13 15:36, Bill Schmidt wrote:
> > > 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?
> > 
> > The issue is not specific to AArch64; please find the attached dumps 
> > generated from the x86-64 gcc by compiling gcc.dg/tree-ssa/slsr-39.c.
> > 
> > W.r.t your comment in the other email about adding a test to verify the 
> > expected gimple, I think the existing test gcc.dg/tree-ssa/slsr-39.c is 
> > sufficient.  The test currently fails on both AArch64 and x86-64, and 
> > presumably also fails on any other 64-bit target where pointer is 64-bit 
> > and int is 32-bit size.  The patch I proposed is to fix this issue and 
> > gcc.dg/tree-ssa/slsr-39.c itself shall be a good regression test (with 
> > specific verification on gimple ir).
> > 
> > The new test proposed in this patch is to regtest the issue my original 
> > patch has, which is a runtime failure due to incorrect optimization.
> > 
> > I'll address other comments in separate emails.
> > 
> > Thanks,
> > Yufeng


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]