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. 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.tkac...@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.tkac...@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.earns...@foss.arm.com] >>>>>> Sent: Thursday, June 04, 2015 4:42 PM >>>>>> To: Kyrill Tkachov; Shiva Chen >>>>>> Cc: Ramana Radhakrishnan; GCC Patches; ni...@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 <shiva0...@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.tkac...@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\"; >>>>>>>>>>>>> } >>>>>>>>>>>>> ) >>>>>>>>>>>>> >