This is the mail archive of the
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: Richard Biener <richard dot guenther at gmail dot com>
- Cc: Yufeng Zhang <Yufeng dot Zhang at arm 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 08:17:55 -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>
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
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,
> > 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.Zhang@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.