From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x531.google.com (mail-pg1-x531.google.com [IPv6:2607:f8b0:4864:20::531]) by sourceware.org (Postfix) with ESMTPS id DB8523858D33 for ; Mon, 13 Feb 2023 06:46:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DB8523858D33 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-pg1-x531.google.com with SMTP id 24so7471176pgt.7 for ; Sun, 12 Feb 2023 22:46:25 -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:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=WbiWJwZ2FeOMkz8v55Fg1b8+4oa9uu0VjtNPxUyKe1Y=; b=nkCfh/TWjzJ68aMcSJ1HCUGhOOHxw39BqmWfAV2blPctwF8ER0ISRra58boMW6qXVs Ba37dwctA9tx5iQbY6jXj+Ye4Z1K577RIYjvpOKGe647IrkmjDwGX2p7R7eepqz/ZEOX qhrqaMnP4BEMb+/L4hCqGKcUnFF0+F+c8cD47qFwggtdPgAVK0Ojs0bPMiRtDWyY9tH8 Kwdxvy8pV1A0QZIB0J75ptj4ev7FFQZs4XCZjHTqPgHRBdc5a2fL9lJyGpaIhI7gIJYi Cfglo9xr8QSh5Y5OQO3d7t+k4LIz3Jh1GrE13c5K2MEepvV7Pfb5i8duRs1OM7RvYxZU mxbQ== 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=WbiWJwZ2FeOMkz8v55Fg1b8+4oa9uu0VjtNPxUyKe1Y=; b=hVkb44tjXHneNFqqj8EJgKcYZ8ba860B19F77HpFIYhFbZvexxTvJAUCz3YgfLrKOp q9bB4CWVmLJn9UzHxkwxwebfnKGGbQTGPGHH+wDGbLPH5g0be/5XwQXj3aiwPYqfawrl 3ms+IX3R+aVnmXGijahDW8eSE5qaAb1jUZqA7AvBhWITs/FX20rMqof6CdBfPa9sK0Jf 7Em1nI6jd7WirbsEgi/rUo+7vt19nIyx2lDOwX7wcjX2VoO66EmNoC13e5TQlD/YpIYt hXBXPmr85DP/E1K7fSpMIo6tSk61WRSj3ENmBFj3zirz9m389pJzrdoG/heIsScksa/y uRvw== X-Gm-Message-State: AO0yUKXi71rekHWO9TKNR7qNeT2FdZX9i7kZBUp25g8UEsETGkNQ+ngd Vtk8jWvpah6xywm5V8WyhJLVLGCRFFQ= X-Google-Smtp-Source: AK7set/ItizCy3ACxeAwb7HiSV3L6W2h43ekk4yBK7txiw7B/htvGQWNvIyuRE0GZUrG8JFFJNBa/A== X-Received: by 2002:a62:5255:0:b0:5a8:bcdc:ed13 with SMTP id g82-20020a625255000000b005a8bcdced13mr1599121pfb.6.1676270783991; Sun, 12 Feb 2023 22:46:23 -0800 (PST) Received: from ?IPV6:2601:681:8600:13d0::f0a? ([2601:681:8600:13d0::f0a]) by smtp.gmail.com with ESMTPSA id x10-20020aa793aa000000b005a8bc154bf4sm1024903pff.39.2023.02.12.22.46.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 12 Feb 2023 22:46:23 -0800 (PST) Message-ID: Date: Sun, 12 Feb 2023 23:46:22 -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: [PATCH] ifcvt: Fix regression in aarch64/fcsel_1.c Content-Language: en-US To: gcc-patches@gcc.gnu.org, richard.sandiford@arm.com References: From: Jeff Law In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,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/3/23 02:15, Richard Sandiford via Gcc-patches wrote: > aarch64/fcsel_1.c contains: > > double > f_2 (double a, double b, double c, double d) > { > if (a > b) > return c; > else > return d; > } > > which started failing in the GCC 12 timeframe. When it passed, > the RTL had the form: > > [A] > (set (reg ret) (reg c)) > (set (pc) (if_then_else (gt ...) (label_ref ret) (pc))) > edge to ret, fallthru to else > else: > (set (reg ret) (reg d)) > fallthru to ret > ret: > ...exit... > > i.e. a branch around. Now the RTL has form: > > [B] > (set (reg ret) (reg d)) > (set (pc) (if_then_else (gt ...) (label_ref then) (pc))) > edge to then, fallthru to ret > ret: > ...exit... > > then: > (set (reg ret) (reg c)) > edge to ret > > i.e. a branch out. > > Both are valid, of course, and there's no easy way to predict > which we'll get. But ifcvt canonicalises its representation on: > > if (cond) goto fallthru else goto non-fallthru > > That is, it canoncalises on the branch-around case for half-diamonds. > It therefore wants to invert the comparison in [B] to get: > > if (...) goto ret else goto then > > But that isn't possible for strict FP gt, so the optimisation fails. > > Canonicalising on the branch-around case seems like the wrong choice for > half diamonds. The natural way of expressing a conditional branch is > for the label_ref to be the "then" destination and pc to be the "else" > destination. And the natural choice of condition seems to be the one > under which extra stuff *is* done, rather than the one under which extra > stuff *isn't* done. But that decision goes back at least 20 years and > it doesn't seem like a good idea to change it in stage 4. As I was parsing things up to the last sentence my first thought was no isn't the right time to fix this :-) > > This patch instead allows the internal structure to store the > condition in inverted form. For simplicity it handles only > conditional moves, which is the one case that is needed > to fix the known regression. (There are probably unknown > regressions too, but still.) > > Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install? > > Richard > > > gcc/ > * ifcvt.h (noce_if_info::cond_inverted): New field. > * ifcvt.cc (cond_move_convert_if_block): Swap the then and else > values when cond_inverted is true. > (noce_find_if_block): Allow the condition to be inverted when > handling conditional moves. > --- > gcc/ifcvt.cc | 31 +++++++++++++++++++------------ > gcc/ifcvt.h | 8 ++++++++ > 2 files changed, 27 insertions(+), 12 deletions(-) > > diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc > index 008796838f7..63ef42b3c34 100644 > --- a/gcc/ifcvt.cc > +++ b/gcc/ifcvt.cc > @@ -4253,6 +4253,9 @@ cond_move_convert_if_block (struct noce_if_info *if_infop, > e = dest; > } > > + if (if_infop->cond_inverted) > + std::swap (t, e); > + > target = noce_emit_cmove (if_infop, dest, code, cond_arg0, cond_arg1, > t, e); > if (!target) > @@ -4405,7 +4408,6 @@ noce_find_if_block (basic_block test_bb, edge then_edge, edge else_edge, > basic_block then_bb, else_bb, join_bb; > bool then_else_reversed = false; > rtx_insn *jump; > - rtx cond; > rtx_insn *cond_earliest; > struct noce_if_info if_info; > bool speed_p = optimize_bb_for_speed_p (test_bb); > @@ -4481,25 +4483,28 @@ noce_find_if_block (basic_block test_bb, edge then_edge, edge else_edge, > if (! onlyjump_p (jump)) > return FALSE; > > - /* If this is not a standard conditional jump, we can't parse it. */ > - cond = noce_get_condition (jump, &cond_earliest, then_else_reversed); > - if (!cond) > - return FALSE; > - > - /* We must be comparing objects whose modes imply the size. */ > - if (GET_MODE (XEXP (cond, 0)) == BLKmode) > - return FALSE; > - > /* Initialize an IF_INFO struct to pass around. */ > memset (&if_info, 0, sizeof if_info); > if_info.test_bb = test_bb; > if_info.then_bb = then_bb; > if_info.else_bb = else_bb; > if_info.join_bb = join_bb; > - if_info.cond = cond; > + if_info.cond = noce_get_condition (jump, &cond_earliest, > + then_else_reversed);; Extraneous ';'. OK with that nit fixed. jeff