From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com [IPv6:2a00:1450:4864:20::12c]) by sourceware.org (Postfix) with ESMTPS id 0E9393858D37 for ; Mon, 9 Jan 2023 07:24:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0E9393858D37 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-lf1-x12c.google.com with SMTP id y25so11596212lfa.9 for ; Sun, 08 Jan 2023 23:24:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=pOz0Se7jq2oSSarRyLBElt+lzYmw4jCiM4JIDMryBQ8=; b=ihA3hVkcE1k5Wdg4zfHwLH4X1yljAMxDgagNSZ1F6QE938objJ87T2zcyxX5eiWKeq /UTZ7NEbjNPZ1ALYrLPPWDEjCb9f67bRjtJvRNd5M0nlNAsfkijQ+alOzmV7KFMje2/F GIFrPPCoqNqKsmwcK36mGvsbxTpdoli7jNw6qBuWdWXnY9JeqHt3sFM06vBBqUo4Wt+N 3dz+bF5bDOyVxI6R97xBgTXOp1XDyyUxSoiM0E4KwwSG33xE5Gs1/Q5kGGb3iLeNdGxr n/WVaZK+x0dZ16exVXpS0mcInnXLCUCbTNbliu7inlJ/qUQ16O3A53L/5ysM8Mp43pmH Y0xg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=pOz0Se7jq2oSSarRyLBElt+lzYmw4jCiM4JIDMryBQ8=; b=6oNGCx1aonOTgBS849p4IOjg48z0xoMkjUhwmcJftdxuPVIf4RfqswWPcs96h44BQb IKyw3LbMBivgafmMoRDhBP7sGiRDzOe+Cgl3JkiIeHLIJ6mM8hzw7tmpzl0eCIDDOosM 7Cr+rxJm8c17v5VavCz23FbIeRfGwHMODPYYXO7L5dF5q6SzrdktihkfjMONXXKOyct+ 8uqWOECbzqakTfm2iqpgLsHGZMcI39gnGFsF1yqjKP53M9CHvubxQbVn2eF1Pv/Nrd3l pzlmRnjYuWaDsuxKjRQ9tow8B1vtZ5j8DdrDvmwU9Zl53WRVGyr8UMwPkjniPgcpJkpf 89Pg== X-Gm-Message-State: AFqh2konLqmYtS/szYYX9/TawKYJ5SPvfjmhdgYOCz56eEwyA5OmwJTV JxekQXkEGE7Z9qJHELfvMABzM+64F2uEpZhAvXE= X-Google-Smtp-Source: AMrXdXtGmvs323e1HEpzHLzYjMvnvF90xBybwhnRGSGzXcZfSg+crSN9g3nd/ZQHphFBblvKhSCM7Aq/uoFxCeiIXtw= X-Received: by 2002:a05:6512:3c88:b0:4b3:a022:2176 with SMTP id h8-20020a0565123c8800b004b3a0222176mr4136774lfv.395.1673249088312; Sun, 08 Jan 2023 23:24:48 -0800 (PST) MIME-Version: 1.0 References: <001001d9165a$73e9cc30$5bbd6490$@nextmovesoftware.com> In-Reply-To: <001001d9165a$73e9cc30$5bbd6490$@nextmovesoftware.com> From: Richard Biener Date: Mon, 9 Jan 2023 08:24:34 +0100 Message-ID: Subject: Re: [x86 PATCH] PR target/106933: Limit TImode STV to SSA-like def-use chains. To: Roger Sayle Cc: GCC Patches , Uros Bizjak , "H.J. Lu" Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,KAM_SHORT,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 Fri, Dec 23, 2022 at 12:10 AM Roger Sayle wrote: > > > With many thanks to H.J. for doing all the hard work, this patch resolves > two P1 regressions; PR target/106933 and PR target/106959. > > Although superficially similar, the i386 backend's two scalar-to-vector > (STV) passes perform their transformations in importantly different ways. > The original pass converting SImode and DImode operations to V4SImode > or V2DImode operations is "soft", allowing values to be maintained in > both integer and vector hard registers. The newer pass converting TImode > operations to V1TImode is "hard" (all or nothing) that converts all uses > of a pseudo to vector form. To implement this it invokes powerful ju-ju > calling SET_MODE on a REG_rtx, which due to RTL sharing, often updates > this pseudo's mode everywhere in the RTL chain. Hence, TImode STV can only > be performed when all uses of a pseudo are convertible to V1TImode form. > To ensure this the STV passes currently use data-flow analysis to inspect > all DEFs and USEs in a chain. This works fine for chains that are in > the usual single assignment form, but the occurrence of uninitialized > variables, or multiple assignments that split a pseudo's usage into > several independent chains (lifetimes) can lead to situations where > some but not all of a pseudo's occurrences need to be updated. This is > safe for the SImode/DImode pass, but leads to the above bugs during > the TImode pass. > > My one minor tweak to HJ's patch from comment #4 of bugzilla PR106959 > is to only perform the new single_def_chain_p check for TImode STV; it > turns out that STV of SImode/DImode min/max operates safely on multiple-def > chains, and prohibiting this leads to testsuite regressions. We don't > (yet) support V1TImode min/max, so this idiom isn't an issue during the > TImode STV pass. > > For the record, the two alternate possible fixes are (i) make the TImode > STV pass "soft", by eliminating use of SET_MODE, instead using replace_rtx > with a new pseudo, or (ii) merging "chains" so that multiple DFA > chains/lifetimes are considered a single STV chain. Similar to (ii) one could also as (iii) split non-overlapping def-use chains to use different pseudos (run web before stv?) In general making the pass "soft" sounds like the right thing to do in the end, if there's cost reasons we want to go all-or-nothing then costing should already ensure that. I agree with Uros that the patch as proposed is best at this stage. Note the single_def_chain_p helper looks like it should be in df*.{cc,h}. There's the related(?) df_find_single_def_src, so a better name could be `df_ref df_single_def (rtx)'? I'll note the function does df_ref adef = DF_REG_DEF_CHAIN (REGNO (reg)); if (adef == NULL || DF_REF_NEXT_REG (adef) != NULL || DF_REF_IS_ARTIFICIAL (adef) || (DF_REF_FLAGS (adef) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL))) return NULL_RTX; in particular it rejects DF_REF_PARTIAL/CONDITIONAL and artificial defs as "single defs" which your function doesn't. Richard. > > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap > and make -k check, both with and without --target_board=unix{-m32}, > with no new failures. Ok for mainline? > > > 2022-12-22 H.J. Lu > Roger Sayle > > gcc/ChangeLog > PR target/106933 > PR target/106959 > * config/i386/i386-features.cc (single_def_chain_p): New predicate > function to check that a pseudo's use-def chain is in SSA form. > (timode_scalar_to_vector_candidate_p): Check that TImode regs that > are SET_DEST or SET_SRC of an insn match/are single_def_chain_p. > > gcc/testsuite/ChangeLog > PR target/106933 > PR target/106959 > * gcc.target/i386/pr106933-1.c: New test case. > * gcc.target/i386/pr106933-2.c: Likewise. > * gcc.target/i386/pr106959-1.c: Likewise. > * gcc.target/i386/pr106959-2.c: Likewise. > * gcc.target/i386/pr106959-3.c: Likewise. > > Thanks in advance, > Roger > -- >