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: Richard Biener <richard dot guenther at gmail dot com>
- To: Yufeng Zhang <Yufeng dot Zhang at arm dot com>
- Cc: Bill Schmidt <wschmidt at linux dot vnet dot ibm 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, 1 Oct 2013 12:19:14 +0200
- 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>
On Wed, Sep 25, 2013 at 1:37 PM, Yufeng Zhang <Yufeng.Zhang@arm.com> wrote:
> 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:
> 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. */
+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.
> * gimple-ssa-strength-reduction.c (safe_to_multiply_p): New
> (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.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>
>>>> Following Bin's patch in
>>>> http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00695.html, this patch
>>>> 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
>>>> 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.