From patchwork Fri Jun 5 13:08:38 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrill Tkachov X-Patchwork-Id: 481194 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id B3AE8140281 for ; Fri, 5 Jun 2015 23:08:57 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=fpnjIy/5; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type:content-transfer-encoding; q=dns; s= default; b=j2JjgJn42dGglBJE4Iq2++V/azcPKt9dLV8ULZByCi4B3VqWX/g7z jrSXKadn27cRGMffes/o3f9JF32RkzJRUQrDwwqHfLIfCLiqAgjXzzw0SX5xekSp v8WImvX2cOHbzf2ZCgX2XlOWnI5jgJ5ydcPesols+16TzVhAxWtcE4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type:content-transfer-encoding; s=default; bh=VkTZxSWZ//9/9XuhfCplMaMRUD0=; b=fpnjIy/5mgjUEhWLyQnQgWxf+CPq VvdjFXO0fAlaF7wT2QWGMrUpELlaNV+aJFEc9IUWIKUwBD9MYFnFO5LAmXq/1tJh LBglyXjjspCWhQZVHsxAK6nMXvYA4EZy8zX9HPqXuTYI+E4+pwxrmenlprl18Q45 0YZ7OAbFoXXcg7k= Received: (qmail 47111 invoked by alias); 5 Jun 2015 13:08:45 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 47090 invoked by uid 89); 5 Jun 2015 13:08:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.5 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; Fri, 05 Jun 2015 13:08:43 +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 12BDD75; Fri, 5 Jun 2015 06:08:48 -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 ADF7F3F447; Fri, 5 Jun 2015 06:08:40 -0700 (PDT) Message-ID: <55719F56.3090003@foss.arm.com> Date: Fri, 05 Jun 2015 14:08:38 +0100 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: Shiva Chen CC: Shiva Chen , Richard Earnshaw , 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> In-Reply-To: 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: > 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\"; >>>>>>>>>>>> } >>>>>>>>>>>> ) >>>>>>>>>>>> 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. Ramana, Richard, we need to backport it to GCC 5 as well, right? Thanks, Kyrill > Thanks, > > Shiva >