From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17439 invoked by alias); 19 Jun 2014 16:30:51 -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 17424 invoked by uid 89); 19 Jun 2014 16:30:50 -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,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: collaborate-mta1.arm.com Received: from fw-tnat.austin.arm.com (HELO collaborate-mta1.arm.com) (217.140.110.23) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 19 Jun 2014 16:30:49 +0000 Received: from [10.1.209.172] (e106919-lin.cambridge.arm.com [10.1.209.172]) by collaborate-mta1.arm.com (Postfix) with ESMTPS id 664B213F884; Thu, 19 Jun 2014 11:30:36 -0500 (CDT) Message-ID: <53A3102B.6020908@arm.com> Date: Thu, 19 Jun 2014 16:30:00 -0000 From: Ramana Radhakrishnan User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Kyrill Tkachov , Marat Zakirov , "gcc-patches@gcc.gnu.org" CC: Gribov Yury , 'Slava Garbuzov' , 'Marat Zakirov' , "tetra2005@gmail.com" , Richard Earnshaw Subject: Re: [PATCH] Fix for PR 61561 References: <000b01cf8bcf$f6e83d20$e4b8b760$@samsung.com> <53A2FDF4.4000304@arm.com> In-Reply-To: <53A2FDF4.4000304@arm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-06/txt/msg01551.txt.bz2 On 19/06/14 16:12, Kyrill Tkachov wrote: > > On 19/06/14 16:05, Marat Zakirov wrote: >> Hi all, >> >> Here's a patch for PR 61561 >> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61561). >> >> It fixes ICE. Thanks for your contribution. However, this is *really* not the way to submit a patch and is the sort of patch that will make me avoid reviewing it instantly. But given this is your first time ...... :) Can you please explain why your fix is required ? One can understand what you are attempting to achieve which is adding constraints and alternatives for moves between sp and general purpose registers for HImode and QImode operands, but how did we get here in the first place ? My next reaction was which pass is doing this and why ? Ah, then I go and read your bug report and your submission there in the description. Now I understand. Please read - https://gcc.gnu.org/contribute.html#patches >> >> Reg. tested on arm15. What's an ARM15 ? I have never seen it or even heard about it. Presumably you mean a Cortex-A15 and that is inferred from your comments in the bugzilla report. What configuration did you test, languages ? Now on to the patch itself. > gcc/ChangeLog: > > 2014-06-19 Marat Zakirov > > * config/arm/arm.md: New templates see pr61561. See Changelog formats and comments about using PR numbers in Changelogs. PR target/61561 * config/arm/arm.md (*movhi_insn_arch4): Handle stack pointer (*arm_movqi_insn): Likewise. > > gcc/testsuite/ChangeLog: > > 2014-06-19 Marat Zakirov > > * c-c++-common/pr61561.c: New test for pr61561. PR target/61561 * : New test. > > > diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md > index 42c12c8..7ed8abc 100644 > --- a/gcc/config/arm/arm.md > +++ b/gcc/config/arm/arm.md > @@ -6290,27 +6290,31 @@ > > ;; Pattern to recognize insn generated default case above > (define_insn "*movhi_insn_arch4" > - [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r") > - (match_operand:HI 1 "general_operand" "rI,K,r,mi"))] > + [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r,r") > + (match_operand:HI 1 "general_operand" "rI,K,r,mi,k"))] > "TARGET_ARM > && arm_arch4 > && (register_operand (operands[0], HImode) > || register_operand (operands[1], HImode))" > - "@ > + "@ > mov%?\\t%0, %1\\t%@ movhi > mvn%?\\t%0, #%B1\\t%@ movhi > str%(h%)\\t%1, %0\\t%@ movhi > - ldr%(h%)\\t%0, %1\\t%@ movhi" > + ldr%(h%)\\t%0, %1\\t%@ movhi > + uxth%?\\t%0, %1\\t%@ movhi" Instead replace the first source constraint with rIk rather than adding another alternative. You don't need a widening operation here. There is no need to do a zero extend here. Any widening or narrowing should be dealt with where required, and the store would remain a store byte and therefore this can be a move like the other moves between registers. This pattern matches for arm_arch4 where uxth is not a valid instruction. Further more on earlier architectures this will cause an undefined instruction trap. > [(set_attr "predicable" "yes") > - (set_attr "pool_range" "*,*,*,256") > - (set_attr "neg_pool_range" "*,*,*,244") > + (set_attr "pool_range" "*,*,*,256,*") > + (set_attr "neg_pool_range" "*,*,*,244,*") > (set_attr_alternative "type" > [(if_then_else (match_operand 1 "const_int_operand" "") > (const_string "mov_imm" ) > (const_string "mov_reg")) > (const_string "mvn_imm") > (const_string "store1") > - (const_string "load1")])] > + (const_string "load1") > + (if_then_else (match_operand 1 "const_int_operand" "") > + (const_string "mov_imm" ) > + (const_string "mov_reg"))])] This should not be needed now. This is just a move_reg. > ) > > (define_insn "*movhi_bytes" > @@ -6429,8 +6433,8 @@ > ) > > (define_insn "*arm_movqi_insn" > - [(set (match_operand:QI 0 "nonimmediate_operand" "=r,r,r,l,r,l,Uu,r,m") > - (match_operand:QI 1 "general_operand" "r,r,I,Py,K,Uu,l,m,r"))] > + [(set (match_operand:QI 0 "nonimmediate_operand" "=r,r,r,l,r,l,Uu,r,m,r,r") > + (match_operand:QI 1 "general_operand" "r,r,I,Py,K,Uu,l,m,r,k,k"))] Why do you need 2 alternatives which appear to do the same thing ? Instead just do a move. > "TARGET_32BIT > && ( register_operand (operands[0], QImode) > || register_operand (operands[1], QImode))" > @@ -6443,12 +6447,14 @@ > ldr%(b%)\\t%0, %1 > str%(b%)\\t%1, %0 > ldr%(b%)\\t%0, %1 > - str%(b%)\\t%1, %0" > - [(set_attr "type" "mov_reg,mov_reg,mov_imm,mov_imm,mvn_imm,load1,store1,load1,store1") > + str%(b%)\\t%1, %0 > + uxtb%?\\t%0, %1 > + uxtb%?\\t%0, %1" > + [(set_attr "type" "mov_reg,mov_reg,mov_imm,mov_imm,mvn_imm,load1,store1,load1,store1,mov_reg,mov_reg") > (set_attr "predicable" "yes") > - (set_attr "predicable_short_it" "yes,yes,yes,no,no,no,no,no,no") > - (set_attr "arch" "t2,any,any,t2,any,t2,t2,any,any") > - (set_attr "length" "2,4,4,2,4,2,2,4,4")] > + (set_attr "predicable_short_it" "yes,yes,yes,no,no,no,no,no,no,no,no") > + (set_attr "arch" "t2,any,any,t2,any,t2,t2,any,any,any,t2") > + (set_attr "length" "2,4,4,2,4,2,2,4,4,4,2")] > ) > > ;; HFmode moves > diff --git a/gcc/testsuite/c-c++-common/pr61561.c b/gcc/testsuite/c-c++-common/pr61561.c > new file mode 100644 > index 0000000..0f4b716 > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/pr61561.c > @@ -0,0 +1,15 @@ > +/* PR c/61561 */ > +/* { dg-do assemble } */ > +/* { dg-options " -w" } */ Typically we put this into gcc.dg/ or if this is a test specific to ARM put it in gcc.target/arm with a dg-options of -O1 / -O2. In this case putting this in gcc.dg is probably ok. Alternatively put this into gcc.c-torture/execute if you want this tortured across multiple optimization levels ? > + > +int dummy(int a); > + > +char a; > +short b; > + > +void mmm (void) > +{ > + char dyn[ dummy(3) ]; > + a = (char)&dyn[0]; > + b = (short)&dyn[0]; > +} Please submit another patch with all these changes again. regards Ramana > > CC'ing the arm maintainers... > > Kyrill > >> >> --Marat >