From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 81183 invoked by alias); 5 Jun 2015 08:29:48 -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 81163 invoked by uid 89); 5 Jun 2015 08:29:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wi0-f172.google.com Received: from mail-wi0-f172.google.com (HELO mail-wi0-f172.google.com) (209.85.212.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 05 Jun 2015 08:29:45 +0000 Received: by wibdq8 with SMTP id dq8so10918900wib.1 for ; Fri, 05 Jun 2015 01:29:42 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.180.105.129 with SMTP id gm1mr57869068wib.51.1433492982015; Fri, 05 Jun 2015 01:29:42 -0700 (PDT) Received: by 10.27.128.196 with HTTP; Fri, 5 Jun 2015 01:29:41 -0700 (PDT) In-Reply-To: <557021DB.5090108@foss.arm.com> 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> Date: Fri, 05 Jun 2015 08:34:00 -0000 Message-ID: Subject: Re: [GCC, ARM] armv8 linux toolchain asan testcase fail due to stl missing conditional code From: Shiva Chen To: Kyrill Tkachov Cc: Shiva Chen , Richard Earnshaw , Ramana Radhakrishnan , GCC Patches , "nickc@redhat.com" Content-Type: multipart/mixed; boundary=f46d04182626fb63990517c11743 X-SW-Source: 2015-06/txt/msg00452.txt.bz2 --f46d04182626fb63990517c11743 Content-Type: text/plain; charset=UTF-8 Content-length: 6759 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 ? 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\"; >>>>>>>>> } >>>>>>>>> ) >>>>>>>>> > --f46d04182626fb63990517c11743 Content-Type: text/plain; charset=US-ASCII; name="Fix_slt_lda_missing_conditional_code.diff" Content-Disposition: attachment; filename="Fix_slt_lda_missing_conditional_code.diff" Content-Transfer-Encoding: base64 X-Attachment-Id: f_iajb7irp0 Content-length: 1733 ZGlmZiAtLWdpdCBhL2djYy9jb25maWcvYXJtL3N5bmMubWQgYi9nY2MvY29u ZmlnL2FybS9zeW5jLm1kCmluZGV4IDQ0Y2RhNjEuLjc1ZGQ1MmUgMTAwNjQ0 Ci0tLSBhL2djYy9jb25maWcvYXJtL3N5bmMubWQKKysrIGIvZ2NjL2NvbmZp Zy9hcm0vc3luYy5tZApAQCAtNzUsMTEgKzc1LDEyIEBACiAgIHsKICAgICBl bnVtIG1lbW1vZGVsIG1vZGVsID0gbWVtbW9kZWxfZnJvbV9pbnQgKElOVFZB TCAob3BlcmFuZHNbMl0pKTsKICAgICBpZiAoaXNfbW1fcmVsYXhlZCAobW9k ZWwpIHx8IGlzX21tX2NvbnN1bWUgKG1vZGVsKSB8fCBpc19tbV9yZWxlYXNl IChtb2RlbCkpCi0gICAgICByZXR1cm4gXCJsZHI8c3luY19zZng+XFx0JTAs ICUxXCI7CisgICAgICByZXR1cm4gXCJsZHIlKDxzeW5jX3NmeD4lKVxcdCUw LCAlMVwiOwogICAgIGVsc2UKLSAgICAgIHJldHVybiBcImxkYTxzeW5jX3Nm eD5cXHQlMCwgJTFcIjsKKyAgICAgIHJldHVybiBcImxkYTxzeW5jX3NmeD4l P1xcdCUwLCAlMVwiOwogICB9Ci0pCisgIFsoc2V0X2F0dHIgInByZWRpY2Fi bGUiICJ5ZXMiKQorICAgKHNldF9hdHRyICJwcmVkaWNhYmxlX3Nob3J0X2l0 IiAibm8iKV0pCiAKIChkZWZpbmVfaW5zbiAiYXRvbWljX3N0b3JlPG1vZGU+ IgogICBbKHNldCAobWF0Y2hfb3BlcmFuZDpRSFNJIDAgIm1lbW9yeV9vcGVy YW5kIiAiPVEiKQpAQCAtOTEsMTEgKzkyLDEyIEBACiAgIHsKICAgICBlbnVt IG1lbW1vZGVsIG1vZGVsID0gbWVtbW9kZWxfZnJvbV9pbnQgKElOVFZBTCAo b3BlcmFuZHNbMl0pKTsKICAgICBpZiAoaXNfbW1fcmVsYXhlZCAobW9kZWwp IHx8IGlzX21tX2NvbnN1bWUgKG1vZGVsKSB8fCBpc19tbV9hY3F1aXJlICht b2RlbCkpCi0gICAgICByZXR1cm4gXCJzdHI8c3luY19zZng+XHQlMSwgJTBc IjsKKyAgICAgIHJldHVybiBcInN0ciUoPHN5bmNfc2Z4PiUpXHQlMSwgJTBc IjsKICAgICBlbHNlCi0gICAgICByZXR1cm4gXCJzdGw8c3luY19zZng+XHQl MSwgJTBcIjsKKyAgICAgIHJldHVybiBcInN0bDxzeW5jX3NmeD4lP1x0JTEs ICUwXCI7CiAgIH0KLSkKKyAgWyhzZXRfYXR0ciAicHJlZGljYWJsZSIgInll cyIpCisgICAoc2V0X2F0dHIgInByZWRpY2FibGVfc2hvcnRfaXQiICJubyIp XSkKIAogOzsgTm90ZSB0aGF0IGxkcmQgYW5kIHZsZHIgYXJlICpub3QqIGd1 YXJhbnRlZWQgdG8gYmUgc2luZ2xlLWNvcHkgYXRvbWljLAogOzsgZXZlbiBm b3IgYSA2NC1iaXQgYWxpZ25lZCBhZGRyZXNzLiAgSW5zdGVhZCB3ZSB1c2Ug YSBsZHJleGQgdW5wYXJpZWQK --f46d04182626fb63990517c11743--