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 ? Thanks, Shiva 2015-06-05 16:34 GMT+08:00 Kyrill Tkachov : > Hi Shiva, > > On 05/06/15 09:29, Shiva Chen wrote: >> >> Hi, Kyrill >> >> I update the patch as Richard's suggestion. >> >> - return \"str\t%1, %0\"; >> + return \"str%(%)\t%1, %0\"; >> else >> - return \"stl\t%1, %0\"; >> + return \"stl%?\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%(%) >> 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 : >>> >>> 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%?\\t%0, %1\"; >>>> else >>>> return \"lda%?\\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 %(>>> The compiler will then put the condition in the correct place. >>>> >>>> So: >>>> >>>> + return \"str%(%)\t%1, %0\"; >>>> >>>> R. >>>> >>>>> Kyrill >>>>> >>>>>> Shiva >>>>>> >>>>>> 2015-06-03 17:29 GMT+08:00 Shiva Chen : >>>>>>> >>>>>>> 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 : >>>>>>>> >>>>>>>> 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\t%1, %0\"; >>>>>>>>>>> + return \"str%?\t%1, %0\"; >>>>>>>>>>> else >>>>>>>>>>> - return \"stl\t%1, %0\"; >>>>>>>>>>> + return \"stl%?\t%1, %0\"; >>>>>>>>>>> } >>>>>>>>>>> ) >>>>>>>>>>> >