[GCC, ARM] armv8 linux toolchain asan testcase fail due to stl missing conditional code
Christophe Lyon
christophe.lyon@linaro.org
Thu Oct 1 20:21:00 GMT 2015
On 1 October 2015 at 11:10, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>
> On 30/09/15 17:39, Kyrill Tkachov wrote:
>>
>> On 09/06/15 09:17, Kyrill Tkachov wrote:
>>>
>>> On 05/06/15 14:14, Kyrill Tkachov wrote:
>>>>
>>>> On 05/06/15 14:11, Richard Earnshaw wrote:
>>>>>
>>>>> On 05/06/15 14:08, Kyrill Tkachov wrote:
>>>>>>
>>>>>> Hi Shiva,
>>>>>>
>>>>>> On 05/06/15 10:42, Shiva Chen wrote:
>>>>>>>
>>>>>>> Hi, Kyrill
>>>>>>>
>>>>>>> I add the testcase as stl-cond.c.
>>>>>>>
>>>>>>> Could you help to check the testcase ?
>>>>>>>
>>>>>>> If it's OK, Could you help me to apply the patch ?
>>>>>>>
>>>>>> This looks ok to me.
>>>>>> One nit on the testcase:
>>>>>>
>>>>>> diff --git a/gcc/testsuite/gcc.target/arm/stl-cond.c
>>>>>> b/gcc/testsuite/gcc.target/arm/stl-cond.c
>>>>>> new file mode 100755
>>>>>> index 0000000..44c6249
>>>>>> --- /dev/null
>>>>>> +++ b/gcc/testsuite/gcc.target/arm/stl-cond.c
>>>>>> @@ -0,0 +1,18 @@
>>>>>> +/* { dg-do compile } */
>>>>>> +/* { dg-require-effective-target arm_arch_v8a_ok } */
>>>>>> +/* { dg-options "-O2" } */
>>>>>>
>>>>>> This should also have -marm as the problem exhibited itself in arm
>>>>>> state.
>>>>>> I'll commit this patch with this change in 24 hours on your behalf if
>>>>>> no
>>>>>> one
>>>>>> objects.
>>>>>>
>>>>> Explicit use of -marm will break multi-lib testing. I've forgotten the
>>>>> correct hook, but there's most-likely something that will give you the
>>>>> right behaviour, even if it means that thumb-only multi-lib testing
>>>>> skips this test.
>>>>
>>>> So I think what we want is:
>>>>
>>>> dg-require-effective-target arm_arm_ok
>>>>
>>>> The comment in target-supports.exp is:
>>>> # Return 1 if this is an ARM target where -marm causes ARM to be
>>>> # used (not Thumb)
>>>>
>>> I've committed the attached patch to trunk on Shiva's behalf with
>>> r224269.
>>> It gates the test on arm_arm_ok and adds -marm, like other similar tests.
>>> The ChangeLog I used is below:
>>
>> I'd like to backport this to GCC 5 and 4.9
>> The patch applies and tests cleanly on GCC 5.
>> On 4.9 it needs some minor changes, which I'm attaching here.
>> I've bootstrapped and tested this patch on 4.9 and the Shiva's
>> original patch on GCC 5.
>>
>> 2015-09-30 Kyrylo Tkachov <kyrylo.tkachov@arm>
>>
>> Backport from mainline
>> 2015-06-09 Shiva Chen <shiva0217@gmail.com>
>>
>> * sync.md (atomic_load<mode>): Add conditional code for lda/ldr
>> (atomic_store<mode>): Likewise.
>>
>> 2015-09-30 Kyrylo Tkachov <kyrylo.tkachov@arm>
>>
>> Backport from mainline
>> 2015-06-09 Shiva Chen <shiva0217@gmail.com>
>>
>> * gcc.target/arm/stl-cond.c: New test.
>>
>>
>> I'll commit them tomorrow.
>
>
> I've now backported the patch to GCC 5 with r228322
> and 4.9 with r228323.
>
Hi Kyrill,
The backport in 4.9 causes build failures in libatomic when GCC is
configured as:
--with-cpu=cortex-a57
--with-fpu=crypto-neon-fp-armv8
--with-mode=arm
--target=arm-none-linux-gnueabihf
For instance when building store_1_.lo:
/tmp/6529147_22.tmpdir/cceUjViw.s:36: Error: bad instruction `stlneb r1,[r0]'
when building load_1_.lo:
/tmp/6529147_22.tmpdir/cchhKmHw.s:37: Error: bad instruction `ldaneb r0,[r0]'
Christophe.
> Kyrill
>
>
>> Thanks,
>> Kyrill
>>
>>
>>
>>> 2015-06-09 Shiva Chen <shiva0217@gmail.com>
>>>
>>> * sync.md (atomic_load<mode>): Add conditional code for lda/ldr
>>> (atomic_store<mode>): Likewise.
>>>
>>> 2015-06-09 Shiva Chen <shiva0217@gmail.com>
>>>
>>> * gcc.target/arm/stl-cond.c: New test.
>>>
>>>
>>> Thanks,
>>> Kyrill
>>>
>>>> Kyrill
>>>>
>>>>
>>>>> R.
>>>>>
>>>>>> Ramana, Richard, we need to backport it to GCC 5 as well, right?
>>>>>>
>>>>>> Thanks,
>>>>>> Kyrill
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Shiva
>>>>>>>
>>>>>>> 2015-06-05 16:34 GMT+08:00 Kyrill Tkachov
>>>>>>> <kyrylo.tkachov@foss.arm.com>:
>>>>>>>>
>>>>>>>> Hi Shiva,
>>>>>>>>
>>>>>>>> On 05/06/15 09:29, Shiva Chen wrote:
>>>>>>>>>
>>>>>>>>> Hi, Kyrill
>>>>>>>>>
>>>>>>>>> I update the patch as Richard's suggestion.
>>>>>>>>>
>>>>>>>>> - return \"str<sync_sfx>\t%1, %0\";
>>>>>>>>> + return \"str%(<sync_sfx>%)\t%1, %0\";
>>>>>>>>> else
>>>>>>>>> - return \"stl<sync_sfx>\t%1, %0\";
>>>>>>>>> + return \"stl<sync_sfx>%?\t%1, %0\";
>>>>>>>>> }
>>>>>>>>> -)
>>>>>>>>> + [(set_attr "predicable" "yes")
>>>>>>>>> + (set_attr "predicable_short_it" "no")])
>>>>>>>>> + [(set_attr "predicable" "yes")
>>>>>>>>> + (set_attr "predicable_short_it" "no")])
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Let me sum up.
>>>>>>>>>
>>>>>>>>> We add predicable attribute to allow gcc do if-conversion in
>>>>>>>>> ce1/ce2/ce3 not only in final phase by final_prescan_insn finite
>>>>>>>>> state
>>>>>>>>> machine.
>>>>>>>>>
>>>>>>>>> We set predicalble_short_it to "no" to restrict conditional code
>>>>>>>>> generation on armv8 with thumb mode.
>>>>>>>>>
>>>>>>>>> However, we could use the flags -mno-restrict-it to force
>>>>>>>>> generating
>>>>>>>>> conditional code on thumb mode.
>>>>>>>>>
>>>>>>>>> Therefore, we have to consider the assembly output format for strb
>>>>>>>>> with condition code on arm/thumb mode.
>>>>>>>>>
>>>>>>>>> Because arm/thumb mode use different syntax for strb,
>>>>>>>>> we output the assembly as str%(<sync_sfx>%)
>>>>>>>>> which will put the condition code in the right place according to
>>>>>>>>> TARGET_UNIFIED_ASM.
>>>>>>>>>
>>>>>>>>> Is there still missing something ?
>>>>>>>>
>>>>>>>> That's all correct, and well summarised :)
>>>>>>>> The patch looks good to me, but please include the testcase
>>>>>>>> (test.c from earlier) appropriately marked up for the testsuite.
>>>>>>>> I think to the level of dg-assemble, just so we know everything is
>>>>>>>> wired up properly.
>>>>>>>>
>>>>>>>> Thanks for dealing with this.
>>>>>>>> Kyrill
>>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Shiva
>>>>>>>>>
>>>>>>>>> 2015-06-04 18:00 GMT+08:00 Kyrill Tkachov
>>>>>>>>> <kyrylo.tkachov@foss.arm.com>:
>>>>>>>>>>
>>>>>>>>>> Hi Shiva,
>>>>>>>>>>
>>>>>>>>>> On 04/06/15 10:57, Shiva Chen wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi, Kyrill
>>>>>>>>>>>
>>>>>>>>>>> Thanks for the tips of syntax.
>>>>>>>>>>>
>>>>>>>>>>> It seems that correct syntax for
>>>>>>>>>>>
>>>>>>>>>>> ldrb with condition code is ldreqb
>>>>>>>>>>>
>>>>>>>>>>> ldab with condition code is ldabeq
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So I modified the pattern as follow
>>>>>>>>>>>
>>>>>>>>>>> {
>>>>>>>>>>> enum memmodel model = (enum memmodel) INTVAL
>>>>>>>>>>> (operands[2]);
>>>>>>>>>>> if (model == MEMMODEL_RELAXED
>>>>>>>>>>> || model == MEMMODEL_CONSUME
>>>>>>>>>>> || model == MEMMODEL_RELEASE)
>>>>>>>>>>> return \"ldr%?<sync_sfx>\\t%0, %1\";
>>>>>>>>>>> else
>>>>>>>>>>> return \"lda<sync_sfx>%?\\t%0, %1\";
>>>>>>>>>>> }
>>>>>>>>>>> [(set_attr "predicable" "yes")
>>>>>>>>>>> (set_attr "predicable_short_it" "no")])
>>>>>>>>>>>
>>>>>>>>>>> It seems we don't have to worry about thumb mode,
>>>>>>>>>>
>>>>>>>>>> I suggest you use Richard's suggestion from:
>>>>>>>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00384.html
>>>>>>>>>> to write this in a clean way.
>>>>>>>>>>
>>>>>>>>>>> Because we already set "predicable" "yes" and
>>>>>>>>>>> predicable_short_it"
>>>>>>>>>>> "no"
>>>>>>>>>>> for the pattern.
>>>>>>>>>>
>>>>>>>>>> That's not quite true. The user may compile for armv8-a with
>>>>>>>>>> -mno-restrict-it which will turn off this
>>>>>>>>>> restriction for Thumb and allow the conditional execution of this.
>>>>>>>>>> In any case, I think Richard's suggestion above should work.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Kyrill
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> The new patch could build gcc and run gcc regression test
>>>>>>>>>>> successfully.
>>>>>>>>>>>
>>>>>>>>>>> Please correct me if I still missing something.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> Shiva
>>>>>>>>>>>
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: Richard Earnshaw [mailto:Richard.Earnshaw@foss.arm.com]
>>>>>>>>>>> Sent: Thursday, June 04, 2015 4:42 PM
>>>>>>>>>>> To: Kyrill Tkachov; Shiva Chen
>>>>>>>>>>> Cc: Ramana Radhakrishnan; GCC Patches; nickc@redhat.com; Shiva
>>>>>>>>>>> Chen
>>>>>>>>>>> Subject: Re: [GCC, ARM] armv8 linux toolchain asan testcase fail
>>>>>>>>>>> due to
>>>>>>>>>>> stl missing conditional code
>>>>>>>>>>>
>>>>>>>>>>> On 04/06/15 09:17, Kyrill Tkachov wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Shiva,
>>>>>>>>>>>>
>>>>>>>>>>>> On 04/06/15 04:13, Shiva Chen wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi, Ramana
>>>>>>>>>>>>>
>>>>>>>>>>>>> Currently, I work for Marvell and the company have copyright
>>>>>>>>>>>>> assignment
>>>>>>>>>>>>> on file.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi, all
>>>>>>>>>>>>>
>>>>>>>>>>>>> After adding the attribute and rebuild gcc, I got the assembler
>>>>>>>>>>>>> error
>>>>>>>>>>>>> message
>>>>>>>>>>>>>
>>>>>>>>>>>>> load_n.s:39: Error: bad instruction `ldrbeq r0,[r0]'
>>>>>>>>>>>>>
>>>>>>>>>>>>> When i look into armv8 ISA document, it seems ldrb Encoding A1
>>>>>>>>>>>>> have
>>>>>>>>>>>>> conditional code field.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Does it mean we should also patch assembler or I just miss
>>>>>>>>>>>>> understanding something ?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Following command use to generate load_n.s:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> /home/shivac/build-system-trunk/Release/build/armv8-marvell-linux-gnu
>>>>>>>>>>>>>
>>>>>>>>>>>>> eabihf-hard/gcc-final/./gcc/cc1 -fpreprocessed load_n.i -quiet
>>>>>>>>>>>>> -dumpbase load_n.c -march=armv8-a -mfloat-abi=hard
>>>>>>>>>>>>> -mfpu=fp-armv8
>>>>>>>>>>>>> -mtls-dialect=gnu -auxbase-strip .libs/load_1_.o -g3 -O2 -Wall
>>>>>>>>>>>>> -Werror -version -fPIC -funwind-tables -o load_n.s
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> The test.c is a simple test case to reproduce missing
>>>>>>>>>>>>> conditional
>>>>>>>>>>>>> code in mmap.c.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Any suggestion ?
>>>>>>>>>>>>
>>>>>>>>>>>> I reproduced the assembler failure with your patch.
>>>>>>>>>>>>
>>>>>>>>>>>> The reason is that for arm mode we use divided syntax, where the
>>>>>>>>>>>> condition field goes in a different place. So, while ldrbeq
>>>>>>>>>>>> r0,[r0] is
>>>>>>>>>>>> rejected, ldreqb r0, [r0] works.
>>>>>>>>>>>> Since we always use divided syntax for arm mode, I think you'll
>>>>>>>>>>>> need
>>>>>>>>>>>> to put the condition field in the right place depending on arm
>>>>>>>>>>>> or
>>>>>>>>>>>> thumb
>>>>>>>>>>>> mode.
>>>>>>>>>>>> Ugh, this is becoming ugly :(
>>>>>>>>>>>>
>>>>>>>>>>> Use %(<suffix%) around the bit that changes for unified/divided
>>>>>>>>>>> syntax.
>>>>>>>>>>> The compiler will then put the condition in the correct
>>>>>>>>>>> place.
>>>>>>>>>>>
>>>>>>>>>>> So:
>>>>>>>>>>>
>>>>>>>>>>> + return \"str%(<sync_sfx>%)\t%1, %0\";
>>>>>>>>>>>
>>>>>>>>>>> R.
>>>>>>>>>>>
>>>>>>>>>>>> Kyrill
>>>>>>>>>>>>
>>>>>>>>>>>>> Shiva
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2015-06-03 17:29 GMT+08:00 Shiva Chen <shiva0217@gmail.com>:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi, Ramana
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I'm not sure what copyright assignment means ?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Does it mean the patch have copyright assignment or not ?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I update the patch to add "predicable" and
>>>>>>>>>>>>>> "predicable_short_it"
>>>>>>>>>>>>>> attribute as suggestion.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> However, I don't have svn write access yet.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Shiva
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2015-06-03 16:36 GMT+08:00 Kyrill Tkachov
>>>>>>>>>>>>>> <kyrylo.tkachov@arm.com>:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 03/06/15 09:32, Ramana Radhakrishnan wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> This pattern is not predicable though, i.e. it doesn't have
>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>> "predicable" attribute set to "yes".
>>>>>>>>>>>>>>>>> Therefore the compiler should be trying to branch around
>>>>>>>>>>>>>>>>> here
>>>>>>>>>>>>>>>>> rather than try to do a cond_exec.
>>>>>>>>>>>>>>>>> Why does the generated code above look like it's converted
>>>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>>> conditional execution?
>>>>>>>>>>>>>>>>> Could you produce a self-contained reduced testcase for
>>>>>>>>>>>>>>>>> this?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> CCFSM state machine in ARM state.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> arm.c (final_prescan_insn).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Ah ok.
>>>>>>>>>>>>>>> This patch makes sense then.
>>>>>>>>>>>>>>> As Ramana mentioned, please mark the pattern with
>>>>>>>>>>>>>>> "predicable"
>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>> also set the "predicable_short_it" attribute to "no" so that
>>>>>>>>>>>>>>> it
>>>>>>>>>>>>>>> will not be conditionalised in Thumb2 mode or when
>>>>>>>>>>>>>>> -mrestrict-it is
>>>>>>>>>>>>>>> enabled.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> Kyrill
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Ramana
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>> Kyrill
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> @@ -91,9 +91,9 @@
>>>>>>>>>>>>>>>>>> {
>>>>>>>>>>>>>>>>>> enum memmodel model = memmodel_from_int
>>>>>>>>>>>>>>>>>> (INTVAL
>>>>>>>>>>>>>>>>>> (operands[2]));
>>>>>>>>>>>>>>>>>> if (is_mm_relaxed (model) || is_mm_consume
>>>>>>>>>>>>>>>>>> (model) ||
>>>>>>>>>>>>>>>>>> is_mm_acquire (model))
>>>>>>>>>>>>>>>>>> - return \"str<sync_sfx>\t%1, %0\";
>>>>>>>>>>>>>>>>>> + return \"str<sync_sfx>%?\t%1, %0\";
>>>>>>>>>>>>>>>>>> else
>>>>>>>>>>>>>>>>>> - return \"stl<sync_sfx>\t%1, %0\";
>>>>>>>>>>>>>>>>>> + return \"stl<sync_sfx>%?\t%1, %0\";
>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>> )
>>>>>>>>>>>>>>>>>>
>
More information about the Gcc-patches
mailing list