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 20:21 -0500, Bill Schmidt wrote:
> On Tue, 2013-10-01 at 23:57 +0100, Yufeng Zhang wrote:
> > On 10/01/13 20:55, Bill Schmidt wrote:
> > >
> > >
> > > 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.
> > 
> > Many thanks for looking into the issue so promptly.  I've updated the 
> > patch; I have to use legal_cast_p_1 instead as the gimple node is no 
> > longer available by then.
> > 
> > Does the new patch look sane?
> 
> Yes, much better.  I'm happy with this approach.  However, please
> restore the correct whitespace before the { at -786,7 +795,7.
> 
> Thanks for fixing this up!
> 
> Bill

(Just a reminder that I can't approve your patch; you need a maintainer
for that.  But it looks good to me.)

Sometime when I get a moment I'm probably going to change this to handle
the casting when the candidates are added to the table.  I think we
should look through the casts and distribute the multiply at that time.
But for now what you have here is good.

Thanks,
Bill

> 
> > 
> > The regtest on aarch64 and bootstrapping on x86-64 are still running.
> > 
> > Thanks,
> > Yufeng
> > 
> > 
> > gcc/
> > 
> > 	* gimple-ssa-strength-reduction.c (legal_cast_p_1): Forward
> > 	declaration.
> > 	(backtrace_base_for_ref): Call get_unwidened with 'base_in' if
> > 	'base_in' represent a conversion and legal_cast_p_1 holds; set
> > 	'base_in' with the returned value from get_unwidened.
> > 
> > gcc/testsuite/
> > 
> > 	* gcc.dg/tree-ssa/slsr-40.c: New test.
> 


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