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 <[email protected]>: > 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 <[email protected]>: >>> >>> 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:[email protected]] >>>> Sent: Thursday, June 04, 2015 4:42 PM >>>> To: Kyrill Tkachov; Shiva Chen >>>> Cc: Ramana Radhakrishnan; GCC Patches; [email protected]; 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 <[email protected]>: >>>>>>> >>>>>>> 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 <[email protected]>: >>>>>>>> >>>>>>>> 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\"; >>>>>>>>>>> } >>>>>>>>>>> ) >>>>>>>>>>> >
diff --git a/gcc/config/arm/sync.md b/gcc/config/arm/sync.md
index 44cda61..75dd52e 100644
--- a/gcc/config/arm/sync.md
+++ b/gcc/config/arm/sync.md
@@ -75,11 +75,12 @@
{
enum memmodel model = memmodel_from_int (INTVAL (operands[2]));
if (is_mm_relaxed (model) || is_mm_consume (model) || is_mm_release
(model))
- 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")
@@ -91,11 +92,12 @@
{
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\";
}
-)
+ [(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 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" } */
+/* { 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" } } */
ChangeLog.fix_slt_lda_missing_conditional_code
Description: Binary data
