From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30788 invoked by alias); 9 Jun 2015 08:17:56 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 30656 invoked by uid 89); 9 Jun 2015 08:17:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.6 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 09 Jun 2015 08:17:52 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5212129; Tue, 9 Jun 2015 01:18:00 -0700 (PDT) Received: from [10.2.207.50] (e100706-lin.cambridge.arm.com [10.2.207.50]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EC0353F32C; Tue, 9 Jun 2015 01:17:48 -0700 (PDT) Message-ID: <5576A12B.80609@foss.arm.com> Date: Tue, 09 Jun 2015 08:44:00 -0000 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Richard Earnshaw , Shiva Chen CC: Shiva Chen , Ramana Radhakrishnan , GCC Patches , "nickc@redhat.com" Subject: Re: [GCC, ARM] armv8 linux toolchain asan testcase fail due to stl missing conditional code References: <556EBB3F.7090603@arm.com> <556EBBAC.2020504@arm.com> <556EBC77.3060601@arm.com> <5570097C.2010706@arm.com> <55700F63.2060700@foss.arm.com> <557021DB.5090108@foss.arm.com> <55715F28.6040800@foss.arm.com> <55719F56.3090003@foss.arm.com> <5571A00F.4040005@foss.arm.com> <5571A0CA.6020002@foss.arm.com> In-Reply-To: <5571A0CA.6020002@foss.arm.com> Content-Type: multipart/mixed; boundary="------------010201070607070207040305" X-SW-Source: 2015-06/txt/msg00637.txt.bz2 This is a multi-part message in MIME format. --------------010201070607070207040305 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Content-length: 10754 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: 2015-06-09 Shiva Chen * sync.md (atomic_load): Add conditional code for lda/ldr (atomic_store): Likewise. 2015-06-09 Shiva Chen * 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 : >>>>> 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 %(>>>>>>> syntax. >>>>>>>> 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\"; >>>>>>>>>>>>>>> } >>>>>>>>>>>>>>> ) >>>>>>>>>>>>>>> > --------------010201070607070207040305 Content-Type: text/x-patch; name="Fix_slt_lda_missing_conditional_code.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="Fix_slt_lda_missing_conditional_code.diff" Content-length: 2004 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 @@ (define_insn "atomic_load" { enum memmodel model = memmodel_from_int (INTVAL (operands[2])); if (is_mm_relaxed (model) || is_mm_consume (model) || is_mm_release (model)) - return \"ldr\\t%0, %1\"; + return \"ldr%(%)\\t%0, %1\"; else - return \"lda\\t%0, %1\"; + return \"lda%?\\t%0, %1\"; } -) + [(set_attr "predicable" "yes") + (set_attr "predicable_short_it" "no")]) (define_insn "atomic_store" [(set (match_operand:QHSI 0 "memory_operand" "=Q") @@ -91,11 +92,12 @@ (define_insn "atomic_store" { 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\"; } -) + [(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" } } */ --------------010201070607070207040305--