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, Because we already set "predicable" "yes" and predicable_short_it" "no" for the pattern. 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\"; >>>>>>> } >>>>>>> ) >>>>>>> >
Fix_slt_lda_missing_conditional_code.diff
Description: Fix_slt_lda_missing_conditional_code.diff