| Message ID | 560C102D.1020801@arm.com |
|---|---|
| State | New |
| Headers | show |
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. 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\"; >>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>> ) >>>>>>>>>>>>>>>>>
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\"; >>>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>>> ) >>>>>>>>>>>>>>>>>> >
commit 685d5dfdfab129bff5892053f420ed48fd40737b
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Wed Sep 30 14:11:01 2015 +0100
[Backport: GCC, ARM] armv8 linux toolchain asan testcase fail due to stl missing conditional code
diff --git a/gcc/config/arm/sync.md b/gcc/config/arm/sync.md
index aa8e9ab..747fc7e 100644
--- a/gcc/config/arm/sync.md
+++ b/gcc/config/arm/sync.md
@@ -77,11 +77,12 @@
if (model == MEMMODEL_RELAXED
|| model == MEMMODEL_CONSUME
|| model == MEMMODEL_RELEASE)
- return \"ldr<sync_sfx>\\t%0, %1\";
+ return \"ldr%(<sync_sfx>%)\\t%0, %1\";
else
- return \"lda<sync_sfx>\\t%0, %1\";
+ return \"lda%(<sync_sfx>%)\\t%0, %1\";
}
-)
+ [(set_attr "predicable" "yes")
+ (set_attr "predicable_short_it" "no")])
(define_insn "atomic_store<mode>"
[(set (match_operand:QHSI 0 "memory_operand" "=Q")
@@ -95,11 +96,12 @@
if (model == MEMMODEL_RELAXED
|| model == MEMMODEL_CONSUME
|| model == MEMMODEL_ACQUIRE)
- 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")])
;; Note that ldrd and vldr are *not* guaranteed to be single-copy atomic,
;; even for a 64-bit aligned address. Instead we use a ldrexd unparied
diff --git a/gcc/testsuite/gcc.target/arm/stl-cond.c b/gcc/testsuite/gcc.target/arm/stl-cond.c
new file mode 100644
index 0000000..de14bb5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/stl-cond.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_arm_ok } */
+/* { dg-require-effective-target arm_arch_v8a_ok } */
+/* { dg-options "-O2 -marm" } */
+/* { dg-add-options arm_arch_v8a } */
+
+struct backtrace_state
+{
+ int threaded;
+ int lock_alloc;
+};
+
+void foo (struct backtrace_state *state)
+{
+ if (state->threaded)
+ __sync_lock_release (&state->lock_alloc);
+}
+
+/* { dg-final { scan-assembler "stlne" } } */