From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x330.google.com (mail-ot1-x330.google.com [IPv6:2607:f8b0:4864:20::330]) by sourceware.org (Postfix) with ESMTPS id 60F0E3858D1E for ; Mon, 30 Oct 2023 23:21:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 60F0E3858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=rivosinc.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rivosinc.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 60F0E3858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::330 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698708121; cv=none; b=q1LMijROUoUXxgfBSBb7O5BN8huP9eyAJEs0ehH05NC+Aq59xVY6m9B1kU7ePnp+a5vHQylPFcC/oiuJYkvvvuahxE0QtRM4xeRvB98vGvxb4rDmDgKzmw+yLuepiGGwMn609seB5Jy4dnMhcBWc2Dur88yqOnJ8VfxH3Imm65o= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698708121; c=relaxed/simple; bh=crf/W+Ioc+k57S1Vo486pWpQj3WOkd3AZ+SY2/BRLC0=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=aWyyCnpgJF25CYuHbGpt2Ud9UahnO4WcleQpluAi5wsqFrXK5Lynh+Vksye7LmkApagQonf9Sh09JP+d9VHwQ/xxwtDEPlyYPEu+iGcRRSWIzl0/MrvISp8hYRYkajDjSilIVju+RHVyykbfAHnw+kMHkrBdQ2waWJnRAGU5cug= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-ot1-x330.google.com with SMTP id 46e09a7af769-6d261cb07dcso3319681a34.3 for ; Mon, 30 Oct 2023 16:21:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20230601.gappssmtp.com; s=20230601; t=1698708118; x=1699312918; darn=gcc.gnu.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=mrorKM75WBtDlRCmjEinaAhqCVAvoWz6vooDoOaqaI8=; b=DEU3vlWYxbDW7tAMyYN71ErJqygShE8gMdU8imEQEWP1UN3j5Sm+V95PTNkF+ifLD6 NS8dp4qCZVLy+YdQkyUKcGZ14zed5Fh/AnkGLLywiVKN5bRhuXF/l3k6nf8tG8G09EOU mJ2VpNrHaxPTLyaT+QVVyYrwmT26KoPRC6d/4P5MHK3NZmOn7h5JSuWzUlMp4P5qutw/ wcRD84P9NfBk4rX8gUC6h9l2I9k96Rg2ems8OPTSKH+NOxqPnn/bQTidIYhsNiYLqhYX d8IewDR9KFyytFoyfYrJDmUrRTyD8q8OUVD52fi+zbCORG/LogowerL4O1iqTqpkeo// VPPQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698708118; x=1699312918; h=content-transfer-encoding:in-reply-to:from:references:cc: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=mrorKM75WBtDlRCmjEinaAhqCVAvoWz6vooDoOaqaI8=; b=buJbMfB3r0L8gkgp1/HVgOTb/r19W+xe/4nPhZ5B5ORvtGaS+ncak1DAS0Rt2nDq6A jz5uJOAyWzbpIs3Fp2YwdyBDwryqc9Hv56No4nIow1ROvxc40jIqNuUTVS5eRvN4SGRh 0ux6TDAcGPVEAe3/sv/0w0RquGi8LUXEfjnHjZcJfoFbRrGccQx8afpfWTCrRw0Zc7RU EXEHPvOsNCvz76kNr17AFpj/xj9FaKm4P7rmniijT6VNasauzLr+52sxAbVD+odGezWg NOxA8rrMDfluGZUusGO2kZtQu/zi8dJvpxa1xkQCBLcb2OL//93ZEHENQ3W4bZ20uydE gVcQ== X-Gm-Message-State: AOJu0Yw3L6926C7g8CD3ky88DQtJSa49l1Pq0xxxGgHYZCJmP5prZ6pK JSyBPRkUYUG5v5XvdLccQ3OXNg== X-Google-Smtp-Source: AGHT+IGT8vH37M37oSfijtq0wM+2BBXOUOi6f5ACSPRRxSIVDz0iliv+yyW5jChXTx1jkkReEKa1SA== X-Received: by 2002:a05:6830:25c1:b0:6bb:1c30:6f3c with SMTP id d1-20020a05683025c100b006bb1c306f3cmr14508514otu.0.1698708118252; Mon, 30 Oct 2023 16:21:58 -0700 (PDT) Received: from [192.168.50.117] (c-73-170-212-163.hsd1.ca.comcast.net. [73.170.212.163]) by smtp.gmail.com with ESMTPSA id bq3-20020a056830388300b006b753685cc5sm14896otb.79.2023.10.30.16.21.57 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 30 Oct 2023 16:21:57 -0700 (PDT) Message-ID: Date: Mon, 30 Oct 2023 16:21:56 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3] RISC-V: elide unnecessary sign extend when expanding cmp_and_jump Content-Language: en-US To: Jeff Law , gcc-patches@gcc.gnu.org Cc: gnu-toolchain@rivosinc.com, Robin Dapp References: <20231030032122.757369-1-vineetg@rivosinc.com> From: Vineet Gupta In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.3 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,URIBL_BLACK 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/30/23 13:33, Jeff Law wrote: > > > On 10/29/23 21:21, Vineet Gupta wrote: >> RV64 compare and branch instructions only support 64-bit operands. >> At Expand time, the backend conservatively zero/sign extends >> its operands even if not needed, such as incoming 32-bit function args >> which ABI/ISA guarantee to be sign-extended already. >> >> And subsequently REE fails to eliminate them as >>     "missing defintion(s)" or "multiple definition(s) >> since function args don't have explicit definition. >> >> So during expand riscv_extend_comparands (), if an operand is a >> subreg-promoted SI with inner DI, which is representative of a function >> arg, just peel away the subreg to expose the DI, eliding the sign >> extension. As Jeff noted this routine is also used in if-conversion so >> also helps there. >> >> Note there's currently patches floating around to improve REE and also a >> new pass to eliminate unneccesary extensions, but it is still beneficial >> to not generate those extra extensions in first place. It is obviously >> less work for post-reload passes such as REE, but even for earlier >> passes, such as combine, having to deal with one less thing and ensuing >> fewer combinations is a win too. >> >> Way too many existing tests used to observe this issue. >> e.g. gcc.c-torture/compile/20190827-1.c -O2 -march=rv64gc >> It elimiates the SEXT.W >> >> Tested with rv64gc with no regressions, I'm relying on PAtrick's >> pre-commit CI to do the full testing. >> >> gcc/ChangeLog: >>     * config/riscv/riscv.cc (riscv_sign_extend_if_not_subreg_prom): New. >>     * (riscv_extend_comparands): Call New function on operands. >> >> Signed-off-by: Vineet Gupta >> --- >> Changes since v2: >>    - Fix linting issues flagged by pre-commit CI >> Changes since v1: >>    - Elide sign extension for 32-bit operarnds only >>    - Apply elison for both arguments >> --- >>   gcc/config/riscv/riscv.cc | 23 +++++++++++++++++++++-- >>   1 file changed, 21 insertions(+), 2 deletions(-) >> >> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc >> index ca9a2ca81d53..269beb3b159b 100644 >> --- a/gcc/config/riscv/riscv.cc >> +++ b/gcc/config/riscv/riscv.cc >> @@ -3678,6 +3678,24 @@ riscv_zero_if_equal (rtx cmp0, rtx cmp1) >>                  cmp0, cmp1, 0, 0, OPTAB_DIRECT); >>   } >>   +/* Helper function for riscv_extend_comparands to Sign-extend the OP. >> +   However if the OP is SI subreg promoted with an inner DI, such as >> +       (subreg/s/v:SI (reg/v:DI) 0 >> +   just peel off the SUBREG to get DI, avoiding extraneous >> extension.  */ >> + >> +static void >> +riscv_sign_extend_if_not_subreg_prom (rtx *op) >> +{ >> +  if (GET_MODE (*op) == SImode >> +      && GET_CODE (*op) == SUBREG >> +      && SUBREG_PROMOTED_VAR_P (*op) >> +      && GET_MODE_SIZE (GET_MODE (XEXP (*op, 0))).to_constant () >> +     == GET_MODE_SIZE (word_mode)) >> +    *op = XEXP (*op, 0); >> +  else >> +    *op = gen_rtx_SIGN_EXTEND (word_mode, *op); > So for the wrapped test GET_MODE_SIZE stuff), add parenthesis and > indent the "==" clause.  ie > >   && (GET_MODE_SIZE (GET_MODE (XEXP (*op), 0))).to_constant () >       == GET_MODE_SIZE (word_mode)) Ok. FWIW I was using the wrong checker: git_check_commit.py vs. check_GNU_style.sh > > Don't you also need to verify that the subreg was sign extended? The > PROMOTED_VAR_P just notes that it was promoted, not *how* it was > promoted.  I think you just need to add a test like this: > >   && SUBREG_PROMOTED_SIGNED_P (*op) Thx for catching this. The orig test case I used to spot the issue had an unsigned promoted subreg but I was convinced it could still be removed (wrong on so many counts). > I don't guess you have data on how this impacts dynamic instruction > counts on anything significant do you? No, haven't run it yet. I can fire one though. I doubt if this is as significant as the prev one, even if this is the right thing to do. > > OK with the formatting nit fixed and adding the additional check to > ensure the value was sign extended. Thx. I just wait for SPEC run before pushing this. -Vineet