From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 90A87385843E for ; Mon, 21 Nov 2022 23:50:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 90A87385843E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 2ALNnbTC028270; Mon, 21 Nov 2022 17:49:37 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 2ALNnbk3028269; Mon, 21 Nov 2022 17:49:37 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Mon, 21 Nov 2022 17:49:37 -0600 From: Segher Boessenkool To: HAO CHEN GUI Cc: David Edelsohn , gcc-patches , "Kewen.Lin" , Peter Bergner Subject: Re: [PATCHv2, rs6000] Enable have_cbranchcc4 on rs6000 Message-ID: <20221121234937.GE25951@gate.crashing.org> References: <438c6628-0b9c-e5d0-e198-2fd6edd16a93@linux.ibm.com> <20221118121822.GY25951@gate.crashing.org> <4056dfc0-57e3-0d13-633c-297572c5c647@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4056dfc0-57e3-0d13-633c-297572c5c647@linux.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,KAM_NUMSUBJECT,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi! On Mon, Nov 21, 2022 at 02:18:39PM +0800, HAO CHEN GUI wrote: > 在 2022/11/18 20:18, Segher Boessenkool 写道: > > I don't think we should pretend we have any conditional jumps the > > machine does not actually have, in cbranchcc4. When would this ever be > > useful? cror;beq can be quite expensive, compared to the code it would > > replace anyway. > > > > If something generates those here (which then ICEs later), that is > > wrong, fix *that*? Is it ifcvt doing it? > > "*cbranch_2insn" is a valid insn for rs6000. So it generates such insn > at expand pass. The "prepare_cmp_insn" called by ifcvt just wants to verify > that the following comparison rtx is valid. *cbranch_2insn is not a machine insn. It generates a cror and a branch insn. This makes no sense to have in a cbranchcc: those do a branch based on an existing cr field, so based on the *output* of that cror. If ifcvt requires differently, ifcvt needs fixing. We want to use the output of the cror multiple times, not generate more cror insns. > (unlt (reg:CCFP 156) > (const_int 0 [0])) > > It should be valid as it's extracted from an existing insn. Why is that an argument? The code is valid, sure, but that doesn't mean we want to generate it all over the place. > It hits ICE only > when the comparison rtx can't pass the predicate check of "cbranchcc4". So > "cbranchcc4" should include "extra_insn_branch_comparison_operator". > > Then, ifcvt tries to call emit_conditional_move_1 to generates a condition > move for FP mode. It definitely fails as there is no conditional move insn for > FP mode in rs6000. The behavior of ifcvt is correct. It tries to do conversion > but fails. It won't hit ICEs after cbranchcc4 is correctly defined. I don't think the behaviour of ifcvt is correct here at all, no. It also does not consider the cost of the code as far as I can see? That could reduce the impact of this problem at least. > Actually, "*cbranch_2insn" has the same logical as float "*cbranch" in ifcvt. > Both of them get a final false return from "rs6000_emit_int_cmove" as rs6000 > doesn't have conditional move for FP mode. I am about to commit patches for that. But only for p10 and later. It should eventually work for everything with isel (setbc can often be optimised to isel after all), but the compiler has to work without isel as well of course. > So I think "cbranchcc4" should include "extra_insn_branch_comparison_operator" > as "*cbranch_2insn" is a valid insn. Just let ifcvt decide a conditional > move is valid or not. It makes a bad decision though. This is not okay. Segher