From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62a.google.com (mail-pl1-x62a.google.com [IPv6:2607:f8b0:4864:20::62a]) by sourceware.org (Postfix) with ESMTPS id 60A493858D33 for ; Mon, 13 Feb 2023 07:31:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 60A493858D33 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-pl1-x62a.google.com with SMTP id r8so12644857pls.2 for ; Sun, 12 Feb 2023 23:31:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; 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=ItIFJ7AfyELSxz6l8buZAlLF33Ev9fpqslAIdHBKOoQ=; b=pODrdzge4PHwAYedA11F012TJ8vLZNbfMpDvGFYmcewKIV6rfuJerktUbYYT+JPx1q J3iCqu6iYuqIAppnOB8DBBiyI/XE0hGBojO+TlyXS8Ooz1mkBjb1HSYerwptjEyXOPin AhhUZrDXQuKho7ASNi0EboksFddsCI5tE/AbAS6AKDfZoE88jkVqPmQlYgpyMaSmPeTv aHdmvHipnxtfWlYfNLMEWM0UgtpGFvnN3VLRHTAfTf2c3oRyZU66mzkgk583F12SP+oZ Mgw0oQA3XbEC43YqVVnz3B4t3CILJTui3g15j2x8LuUpYo4cI6VE+bi0XofC6tvMMYzV oQww== 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: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=ItIFJ7AfyELSxz6l8buZAlLF33Ev9fpqslAIdHBKOoQ=; b=mRnYxO35aez4og1TDsSo8ABnO+E/0Z2RLg99VHY8MDrfYSksA36PTJu7q8aJAPNi1x +XdksCnlFVTFxAV77mtNtISrjQwE7IRYY1IHU6GS4f7dmi/cYkyYRqSJ9oIBnjyTDLx0 kgJslQOiehx+6OIeNHvJg0mQdhz+rfRBcLn7UrH21SM3t0lDqYFtVJKMOk+kIhhPq9gU UrLpw1pH1wIU4IF+IVKaOyPLCvEHuGfBSC76uqNS/2epP1pvErpypxsevJr0ZNoSckTF S2bBZj2UJKzj9oaqC5Cc14qtyixz1opWiP5qgF9aKP01I4TcQMI11L2yd+/WJVmn2feV UJdA== X-Gm-Message-State: AO0yUKU0QuZ+kRQ5zXbMcNoZXjf4JbFOwL/ViSzsSCxfw2tanHVxDl+V wFRKdloit5B3OIE99r2ciyU= X-Google-Smtp-Source: AK7set83qGMsVI7WH+fcSYWJOpxzwZs+WbXnZdMgADo7f4YTk3Wxj1XN+Q+XOTFYzfIkM3XEBLrx7w== X-Received: by 2002:a17:903:2284:b0:19a:5939:51dd with SMTP id b4-20020a170903228400b0019a593951ddmr16022983plh.20.1676273507212; Sun, 12 Feb 2023 23:31:47 -0800 (PST) Received: from ?IPV6:2601:681:8600:13d0::f0a? ([2601:681:8600:13d0::f0a]) by smtp.gmail.com with ESMTPSA id jj13-20020a170903048d00b0019a5aa7eab0sm7456691plb.54.2023.02.12.23.31.45 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 12 Feb 2023 23:31:46 -0800 (PST) Message-ID: <90ef6883-397b-8e6a-1ecb-3663fe854ac7@gmail.com> Date: Mon, 13 Feb 2023 00:31:45 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [RFC PATCH v1 08/10] ifcvt: add if-conversion to conditional-zero instructions Content-Language: en-US To: Philipp Tomsich , gcc-patches@gcc.gnu.org Cc: Kito Cheng , Christoph Muellner , Palmer Dabbelt , Andrew Waterman , Vineet Gupta References: <20230210224150.2801962-1-philipp.tomsich@vrull.eu> <20230210224150.2801962-9-philipp.tomsich@vrull.eu> From: Jeff Law In-Reply-To: <20230210224150.2801962-9-philipp.tomsich@vrull.eu> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,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 2/10/23 15:41, Philipp Tomsich wrote: > Some architectures, as it the case on RISC-V with the proposed > ZiCondOps and the vendor-defined XVentanaCondOps, define a > conditional-zero instruction that is equivalent to: > - the positive form: rd = (rc != 0) ? rs : 0 > - the negated form: rd = (rc == 0) ? rs : 0 > > While noce_try_store_flag_mask will somewhat work for this case, it > will generate a number of atomic RTX that will misdirect the cost > calculation and may be too long (i.e., 4 RTX and more) to successfully > merge at combine-time. > > Instead, we add two new transforms that attempt to build up what we > define as the canonical form of a conditional-zero expression: > > (set (match_operand 0 "register_operand" "=r") > (and (neg (eq_or_ne (match_operand 1 "register_operand" "r") > (const_int 0))) > (match_operand 2 "register_operand" "r"))) > > Architectures that provide a conditional-zero are thus expected to > define an instruction matching this pattern in their backend. > > Based on this, we support the following cases: > - noce_try_condzero: > a ? a : b > a ? b : 0 (and then/else swapped) > !a ? b : 0 (and then/else swapped) > - noce_try_condzero_arith: > conditional-plus, conditional-minus, conditional-and, > conditional-or, conditional-xor, conditional-shift, > conditional-and > > Given that this is hooked into the CE passes, it is less powerful than > a tree-pass (e.g., it can not transform cases where an extension, such > as for uint16_t operations is in either the then or else-branch > together with the arithmetic) but already covers a good array of cases > and triggers across SPEC CPU 2017. > > Adding transformations in a tree pass should come in a future > improvement. > > gcc/ChangeLog: > > * ifcvt.cc (noce_emit_insn): Add prototype. > (noce_emit_condzero): Helper for noce_try_condzero and > noce_try_condzero_arith transforms. > (noce_try_condzero): New transform. > (noce_try_condzero_arith): New transform for conditional > arithmetic that can be built up by exploiting that the > conditional-zero instruction will inject 0, which acts > as the neutral element for operations. > (noce_process_if_block): Call noce_try_condzero and > noce_try_condzero_arith. A few things we've noted while working with this patch internally. 1. The canonical conditional-zero-or-value requires operand 2 to be a register. That will tend to inhibit if-conversion of something like a shift-by-constant. This showed up somewhere in leela. 2. Internally we fixed #1 by forcing that argument into a register when it was a constant and reload hasn't completed. This (naturally) resulted in more if-conversions happening. That in turn caused perlbench to fail. This was tracked down to a conditional-and sequence. I think the call to noce_emit_condzero needs adjustment to pass arg0 instead of arg1 when OP == AND. 3. The canaonical conditional-zero-or-value assumes the target can do a generic SEQ/SNE of two register values. As you know, on RISC-V we have SEQZ/SNEZ. So we've added another fallback path to handle that case in noce_emit_condzero. You subtract the two values, then you can do an SEQZ/SNEZ on the result. Formatting nits noted below. > + if (!noce_emit_insn (gen_rtx_SET (tmp, cond))) { > + end_sequence (); > + return NULL_RTX; > + } Open-curly on new line, indented two places from the IF, similarly for the close curly. That will probably require adjusting the indentation of the statements. > + if (code == NE && cond_arg1 == const0_rtx && > + REG_P (b) && rtx_equal_p (b, cond_arg0)) Don't end with &&, instead bring it down indented one position from the open parenthesis. > + { > + orig_b = b; > + b = const0_rtx; > + } > + > + /* We may encounter the form "(b == 0) ? b : a", which can be > + simplied to "(b == 0) ? 0 : a". */ > + if (code == EQ && cond_arg1 == const0_rtx && > + REG_P (b) && rtx_equal_p (b, cond_arg0)) Likewise. > + { > + target = noce_emit_condzero(if_info, reversep ? a : b, reversep); Missing whitespace before open-paren of function call. > + > + if (op != PLUS && op != MINUS && op != IOR && op != XOR && > + op != ASHIFT && op != ASHIFTRT && op != LSHIFTRT && op != AND) Bring the "&&" down to the next line here too. > + > + if (!rtx_equal_p (if_info->x, arg0)) > + return FALSE; ?!? What's this for? > + > + target = noce_emit_condzero(if_info, arg1, op != AND ? true : false); Missing space before open-paren. > + > + emit_insn_before_setloc(seq, if_info->jump, > + INSN_LOCATION(if_info->insn_a)); Here too. I think the ChangeLog entry for the testsuite needs its filenames updated :-) While this was submitted before stage1 closed, I think we're probably too late for it to go forward until we re-open stage1. jeff