This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH GCC] Tweak gimple-ssa-strength-reduction.c:backtrace_base_for_ref () to cover different cases as seen on AArch64
- From: Bill Schmidt <wschmidt at linux dot vnet dot ibm dot com>
- To: Yufeng Zhang <Yufeng dot Zhang at arm dot com>
- Cc: Richard Biener <richard dot guenther at gmail dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Bin Cheng <Bin dot Cheng at arm dot com>, Dominique Dhumieres <dominiq at lps dot ens dot fr>
- Date: Tue, 01 Oct 2013 20:21:54 -0500
- Subject: Re: [PATCH GCC] Tweak gimple-ssa-strength-reduction.c:backtrace_base_for_ref () to cover different cases as seen on AArch64
- Authentication-results: sourceware.org; auth=none
- References: <522F4092 dot 1040100 at arm dot com> <CAFiYyc2vk5AczEHFy=etHkB_aCKhOziamYj7kRxmpnAcb461VA at mail dot gmail dot com> <1378903160 dot 3730 dot 38 dot camel at gnopaine> <5242CAEE dot 5010700 at arm dot com> <CAFiYyc0jTK8bso1vPWn2ffgvLav1BScv6JhQi5gFvguKFaouAQ at mail dot gmail dot com> <1380633475 dot 367 dot 70 dot camel at gnopaine> <1380638208 dot 367 dot 73 dot camel at gnopaine> <524AE4ED dot 5080404 at arm dot com> <1380646569 dot 367 dot 96 dot camel at gnopaine> <1380657305 dot 367 dot 99 dot camel at gnopaine> <524B5353 dot 1000509 at arm dot com>
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
>
> 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.