From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24515 invoked by alias); 24 Nov 2014 05:28:42 -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 24503 invoked by uid 89); 24 Nov 2014 05:28:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-la0-f49.google.com Received: from mail-la0-f49.google.com (HELO mail-la0-f49.google.com) (209.85.215.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 24 Nov 2014 05:28:40 +0000 Received: by mail-la0-f49.google.com with SMTP id hs14so6978610lab.36 for ; Sun, 23 Nov 2014 21:28:37 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.152.25.226 with SMTP id f2mr16776593lag.98.1416806917378; Sun, 23 Nov 2014 21:28:37 -0800 (PST) Received: by 10.25.6.4 with HTTP; Sun, 23 Nov 2014 21:28:37 -0800 (PST) In-Reply-To: <000101d007a5$0d120f30$27362d90$@arm.com> References: <000101d007a5$0d120f30$27362d90$@arm.com> Date: Mon, 24 Nov 2014 08:21:00 -0000 Message-ID: Subject: Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015) From: Andrew Pinski To: Zhenqiang Chen Cc: GCC Patches , Marcus Shawcroft Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-11/txt/msg02970.txt.bz2 On Sun, Nov 23, 2014 at 9:11 PM, Zhenqiang Chen wrote: > Hi, > > Expand pass always uses sign-extend to represent constant value. For the > case in the patch, a 8-bit unsigned value "252" is represented as "-4", > which pass the ccmn check. After mode conversion, "-4" becomes "252", which > leads to mismatch. > > The patch adds another operand check after mode conversion. > > No make check regression with qemu. > > OK for trunk? > > Thanks! > -Zhenqiang > > ChangeLog: > 2014-11-24 Zhenqiang Chen > > PR target/64015 > * config/aarch64/aarch64.c (aarch64_gen_ccmp_first): Recheck operand > after mode conversion. > (aarch64_gen_ccmp_next): Likewise. > > testsuite/ChangeLog: > 2014-11-24 Zhenqiang Chen > > * gcc.target/aarch64/pr64015.c: New test. > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index 1809513..203d095 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -10311,7 +10311,10 @@ aarch64_gen_ccmp_first (int code, rtx op0, rtx op1) > if (!aarch64_plus_operand (op1, GET_MODE (op1))) > op1 = force_reg (mode, op1); > > - if (!aarch64_convert_mode (&op0, &op1, unsignedp)) > + if (!aarch64_convert_mode (&op0, &op1, unsignedp) > + /* Some negative value might be transformed into a positive one. > + So need recheck here. */ > + || !aarch64_plus_operand (op1, GET_MODE (op1))) > return NULL_RTX; Shouldn't we force it to a reg here instead? > > mode = aarch64_code_to_ccmode ((enum rtx_code) code); > @@ -10344,7 +10347,10 @@ aarch64_gen_ccmp_next (rtx prev, int cmp_code, rtx > op0, rtx op1, int bit_code) > || !aarch64_ccmp_operand (op1, GET_MODE (op1))) > return NULL_RTX; > > - if (!aarch64_convert_mode (&op0, &op1, unsignedp)) > + if (!aarch64_convert_mode (&op0, &op1, unsignedp) > + /* Some negative value might be transformed into a positive one. > + So need recheck here. */ > + || !aarch64_ccmp_operand (op1, GET_MODE (op1))) > return NULL_RTX; Also really the cost of forcing to a register is better really. In the cases where we would not have forced to a register in a cmp instruction, the constant would be one instruction and compared to the cost of two cset and an and/or is better. In the cases where we would have forced to a register for the cmp instruction, two cost for doing the forcing is the same on both cases but since we gaining from removing a cset and an and/or we are better. > > mode = aarch64_code_to_ccmode ((enum rtx_code) cmp_code); > > diff --git a/gcc/testsuite/gcc.target/aarch64/pr64015.c > b/gcc/testsuite/gcc.target/aarch64/pr64015.c > new file mode 100644 > index 0000000..eeed665 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/pr64015.c > @@ -0,0 +1,7 @@ > +/* { dg-do compile } */ > +/* { dg-options " -O2 " } */ > +int > +test (unsigned short a, unsigned char b) > +{ > + return a > 0xfff2 && b > 252; > +} Since this testcase is generic (except for the -O2), it really should go into gcc.c-torture/compile instead of remove the two dg-* directives so it can be tested on more than AARCH64 and on more optimization levels. Thanks, Andrew Pinski > > >