From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x42c.google.com (mail-pf1-x42c.google.com [IPv6:2607:f8b0:4864:20::42c]) by sourceware.org (Postfix) with ESMTPS id D425A3858290 for ; Wed, 12 Oct 2022 17:57:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D425A3858290 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-pf1-x42c.google.com with SMTP id g28so17197257pfk.8 for ; Wed, 12 Oct 2022 10:57:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=wQ63sEY4MZHmEaeeazLChcxZGMlUqOcrQkq+Q8hye2A=; b=dtMge5IIE/ybHTxSWtb8lR2drrSeAai5CnJiwC3ufl1C0ZfuP/Ao1DYDV2TL4+Jowi 7opIXoRbDdMgtxVsouqAm6Yecs+1VfIiImNZtIC2JyEemUoTb9eZd4Fw8MrpJfnBDS6l 2i109MjW8pQuSaY+BIlAwT2SRgr+cEomB2+INzHU4CEtM84WTik5n05FS2GPNCeo6550 Kgt2vuVFnxN6HqAFRphBvdN6jnWCd580fSmgCP6RSAXagtsdIrIxakRacE51A59V+q3N o1Q2m3LbGLAZvcgAlVf/NxwZFxCpsVdsO4S/jXK5UAfvaO5VVkBaykzhPBPlT75sYKwr XFZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=wQ63sEY4MZHmEaeeazLChcxZGMlUqOcrQkq+Q8hye2A=; b=ITMRWUUi+ZQgdXqARUvQfMah/2OycFtyuUlDX5HlVPq1/MveOgsv2Pb7Jf2+rJ5KFB olVpG1z9zRVCcVQDJoYvx9ktofFiqABeHQ0v590zSbbUspD1SFuZEk7KbEjRUC3URRLW c54FWbStD+PXbmI8mNZ2sP+LEvPmqzaKJM5uyChaPb31jrMEWqUxMm6tsMjGLkY0RQWs jlea2hYS92l5T2Epk4ghhiBeOf+mo1YGrz0iN2tAnJoCDH94Iv0H7tJ6A1efw29/yvAb ducc3pHUucVCJKPOmKHYISum0A852c0qJpNbEgbDBJ1Q/UNXQQIWlnOb1+JVcPpQ0NIl hqkw== X-Gm-Message-State: ACrzQf06wp3SHZGzmMzk8f1esySCT3HMGYtdKGMsW/sUvrV+QrQXej49 V3l6Lwb4Rleq44AaFt6QDW0uu7WtwMSEhg== X-Google-Smtp-Source: AMsMyM52SjoGKsxtbz/GM/0Hdz+Z1Yk7XxHKzi/+fpBY5mSjysqmcGoda4JFNI3Q/Ns61kMtRpGcrg== X-Received: by 2002:a05:6a00:2185:b0:520:7276:6570 with SMTP id h5-20020a056a00218500b0052072766570mr32088308pfi.84.1665597459294; Wed, 12 Oct 2022 10:57:39 -0700 (PDT) Received: from ?IPV6:2601:681:8600:13d0::f0a? ([2601:681:8600:13d0::f0a]) by smtp.gmail.com with ESMTPSA id g139-20020a625291000000b00562eff85594sm155190pfb.121.2022.10.12.10.57.37 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 12 Oct 2022 10:57:38 -0700 (PDT) Message-ID: <04cf00ea-8aa7-efcc-6553-14e50461d2b5@gmail.com> Date: Wed, 12 Oct 2022 11:57:34 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 Subject: Re: [PATCH] improved const shifts for AVR targets Content-Language: en-US To: gcc-patches@gcc.gnu.org, Alexander Binzberger References: From: Jeff Law In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 10/4/22 11:06, Alexander Binzberger via Gcc-patches wrote: > Hi, > recently I used some arduino uno for a project and realized some areas > which do not output optimal asm code. Especially around shifts and function > calls. > With this as motivation and hacktoberfest I started patching things. > Since patch files do not provide a good overview and I hope for a > "hacktoberfest-accepted" label on the PR on github I also opened it there: > https://github.com/gcc-mirror/gcc/pull/73 > > This patch improves shifts with const right hand operand. While 8bit and > 16bit shifts where mostly fine 24bit and 32bit where not handled well. > > Testing > I checked output with a local installation of compiler explorer in asm and > a tiny unit test comparing shifts with mul/div by 2. > I however did not write any testcases in gcc for it. > > Target > This patch is only targeting atmel avr family of chips. > > Changelog > improved const shifts for AVR targets It would be helpful if you could show the before/after code for the cases you're changing.  Extra credit if you include cycles & size information for those cases.  That would help someone like me who knows GCC well, but isn't particularly well versed in the AVR target evaluate the overarching goal of the patch (ie, better code). Changes should include a ChangeLog which indicates what changed. If you look at git log you will see examples of what a ChangeLog should look like. The is large enough that you need either a  copyright assignment or DCO certification. See this page for details: https://gcc.gnu.org/contribute.html > > Patch > ----- > diff --git a/gcc/config/avr/avr.cc b/gcc/config/avr/avr.cc > index 4ed390e4cf9..c7b70812d5c 100644 > --- a/gcc/config/avr/avr.cc > +++ b/gcc/config/avr/avr.cc > @@ -6043,9 +6043,6 @@ out_shift_with_cnt (const char *templ, rtx_insn > *insn, rtx operands[], > op[2] = operands[2]; > op[3] = operands[3]; > > - if (plen) > - *plen = 0; > - Doesn't this leave *plen uninitialized for the case where the shift count is held in memory or a register or is an out of range constant?  Is this really safe? > if (CONST_INT_P (operands[2])) > { > /* Operand 3 is a scratch register if this is a > @@ -6150,96 +6147,68 @@ out_shift_with_cnt (const char *templ, rtx_insn > *insn, rtx operands[], > /* 8bit shift left ((char)x << i) */ > > const char * > -ashlqi3_out (rtx_insn *insn, rtx operands[], int *len) > +ashlqi3_out (rtx_insn *insn, rtx operands[], int *plen) > { > if (CONST_INT_P (operands[2])) > { > - int k; > - > - if (!len) > - len = &k; > - Isn't this wrong for the call to ashlqi3_out from avr.md?  In that call len/plen will be zero, which we then pass down.  So the question is why did you remove this code? The patch as-is is relatively large and can easily be broken down into more manageable chunks.  I would suggest a patch for each mode.  ie, one which changes QImode shifts, another for HImode shifts, another for PSImode shifts. etc.  It may seem like more work, but by breaking it down reviewers can take action on each patch individually.  So for example its relatively easy to work through the QImode changes and those could go in fairly quick while the PSImode changes will require considerably more time to review. > switch (INTVAL (operands[2])) > { > default: > if (INTVAL (operands[2]) < 8) > break; > > - *len = 1; > - return "clr %0"; > - > - case 1: > - *len = 1; > - return "lsl %0"; > - > - case 2: > - *len = 2; > - return ("lsl %0" CR_TAB > - "lsl %0"); > - > - case 3: > - *len = 3; > - return ("lsl %0" CR_TAB > - "lsl %0" CR_TAB > - "lsl %0"); > + return avr_asm_len ("clr %0", operands, plen, 1); You've probably got a whitespace problem here.  I think the return should line up in the came column as the IF statement. Conceptually this seems reasonable as cases 1, 2 and 3 can be trivially handled by out_shift_with_cnt.  Tough routing more code through out_shift_with_cnt means the comment might need to change since we're routing more cases through it that were trivially handled in ashlqi3_out before. > > case 4: > if (test_hard_reg_class (LD_REGS, operands[0])) > { > - *len = 2; > - return ("swap %0" CR_TAB > - "andi %0,0xf0"); > + return avr_asm_len ("swap %0" CR_TAB > + "andi %0,0xf0", operands, plen, 2); More indention problems here.  THe return should line up two spaces inside the open curly brace.  Otherwise this case seems reasonable since it's generating the same code as before. > } > - *len = 4; > - return ("lsl %0" CR_TAB > + return avr_asm_len ("lsl %0" CR_TAB > "lsl %0" CR_TAB > "lsl %0" CR_TAB > - "lsl %0"); > + "lsl %0", operands, plen, 4); Gratuitous indentation changes.  Please don't do that unless you're fixing cases where the indentation is wrong according to GNU/project standards. > > case 5: > if (test_hard_reg_class (LD_REGS, operands[0])) > { > - *len = 3; > - return ("swap %0" CR_TAB > + return avr_asm_len ("swap %0" CR_TAB > "lsl %0" CR_TAB > - "andi %0,0xe0"); > + "andi %0,0xe0", operands, plen, 3); It looks like you're introducing gratuitous indentation changes here.  Please don't do that.  Otherwise this looks sensible as again, it doesn't change the generated code. > } > - *len = 5; > - return ("lsl %0" CR_TAB > + return avr_asm_len ("lsl %0" CR_TAB > "lsl %0" CR_TAB > "lsl %0" CR_TAB > "lsl %0" CR_TAB > - "lsl %0"); > + "lsl %0", operands, plen, 5); Similarly. > > case 6: > if (test_hard_reg_class (LD_REGS, operands[0])) > { > - *len = 4; > - return ("swap %0" CR_TAB > + return avr_asm_len ("swap %0" CR_TAB > "lsl %0" CR_TAB > "lsl %0" CR_TAB > - "andi %0,0xc0"); > + "andi %0,0xc0", operands, plen, 4); > } > - *len = 6; > - return ("lsl %0" CR_TAB > + return avr_asm_len ("lsl %0" CR_TAB > "lsl %0" CR_TAB > "lsl %0" CR_TAB > "lsl %0" CR_TAB > "lsl %0" CR_TAB > - "lsl %0"); > + "lsl %0", operands, plen, 6); > > case 7: > - *len = 3; > - return ("ror %0" CR_TAB > + return avr_asm_len ("ror %0" CR_TAB > "clr %0" CR_TAB > - "ror %0"); > + "ror %0", operands, plen, 3); > } Similarly for these cases. I don't have the time right now to go through the other modes. But I would suggest you take the feedback above and apply the same concepts to the changes for the other modes. Note for other reviewers, without an assignment or DCO cert, this can't go forward. jeff