From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 125754 invoked by alias); 5 Jun 2015 09:42:12 -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 125745 invoked by uid 89); 5 Jun 2015 09:42:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 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-f179.google.com Received: from mail-wi0-f179.google.com (HELO mail-wi0-f179.google.com) (209.85.212.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 05 Jun 2015 09:42:09 +0000 Received: by wibut5 with SMTP id ut5so15251200wib.1 for ; Fri, 05 Jun 2015 02:42:06 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.194.21.232 with SMTP id y8mr5034172wje.36.1433497326014; Fri, 05 Jun 2015 02:42:06 -0700 (PDT) Received: by 10.27.128.196 with HTTP; Fri, 5 Jun 2015 02:42:05 -0700 (PDT) In-Reply-To: <55715F28.6040800@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> <55715F28.6040800@foss.arm.com> Date: Fri, 05 Jun 2015 10:59: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=047d7b5d5908e7c7210517c21a81 X-SW-Source: 2015-06/txt/msg00456.txt.bz2 --047d7b5d5908e7c7210517c21a81 Content-Type: text/plain; charset=UTF-8 Content-length: 7875 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 : > 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\"; >>>>>>>>>>> } >>>>>>>>>>> ) >>>>>>>>>>> > --047d7b5d5908e7c7210517c21a81 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_iajf20sh1 Content-length: 2550 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 YSBsZHJleGQgdW5wYXJpZWQKZGlmZiAtLWdpdCBhL2djYy90ZXN0c3VpdGUv Z2NjLnRhcmdldC9hcm0vc3RsLWNvbmQuYyBiL2djYy90ZXN0c3VpdGUvZ2Nj LnRhcmdldC9hcm0vc3RsLWNvbmQuYwpuZXcgZmlsZSBtb2RlIDEwMDc1NQpp bmRleCAwMDAwMDAwLi40NGM2MjQ5Ci0tLSAvZGV2L251bGwKKysrIGIvZ2Nj L3Rlc3RzdWl0ZS9nY2MudGFyZ2V0L2FybS9zdGwtY29uZC5jCkBAIC0wLDAg KzEsMTggQEAKKy8qIHsgZGctZG8gY29tcGlsZSB9ICovCisvKiB7IGRnLXJl cXVpcmUtZWZmZWN0aXZlLXRhcmdldCBhcm1fYXJjaF92OGFfb2sgfSAqLwor LyogeyBkZy1vcHRpb25zICItTzIiIH0gKi8KKy8qIHsgZGctYWRkLW9wdGlv bnMgYXJtX2FyY2hfdjhhIH0gKi8KKworc3RydWN0IGJhY2t0cmFjZV9zdGF0 ZQoreworICBpbnQgdGhyZWFkZWQ7CisgIGludCBsb2NrX2FsbG9jOworfTsK Kwordm9pZCBmb28gKHN0cnVjdCBiYWNrdHJhY2Vfc3RhdGUgKnN0YXRlKQor eworICBpZiAoc3RhdGUtPnRocmVhZGVkKQorICAgIF9fc3luY19sb2NrX3Jl bGVhc2UgKCZzdGF0ZS0+bG9ja19hbGxvYyk7Cit9CisKKy8qIHsgZGctZmlu YWwgeyBzY2FuLWFzc2VtYmxlciAic3RsbmUiIH0gfSAqLwo= --047d7b5d5908e7c7210517c21a81 Content-Type: application/octet-stream; name="ChangeLog.fix_slt_lda_missing_conditional_code" Content-Disposition: attachment; filename="ChangeLog.fix_slt_lda_missing_conditional_code" Content-Transfer-Encoding: base64 X-Attachment-Id: f_iajf20ry0 Content-length: 249 MjAxNS0wNi0wNSAgU2hpdmEgQ2hlbiAgPHNoaXZhMDIxN0BnbWFpbC5jb20+ CgoJKiBjb25maWcvYXJtL3N5bmMubWQgKGF0b21pY19sb2FkPG1vZGU+LChh dG9taWNfc3RvcmU8bW9kZT4pOiAKCUFkZCBjb25kaXRpb24gY29kZSBmb3Ig YXJtIGxvYWQgYWNxdWlyZS9zdG9yZSByZWxlYXNlIGluc3RydWN0aW9ucwoK Cg== --047d7b5d5908e7c7210517c21a81--