From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 42FC53858D28 for ; Fri, 12 Aug 2022 11:31:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 42FC53858D28 Received: from mail-oi1-f199.google.com (mail-oi1-f199.google.com [209.85.167.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-593-TkDx0wuPO0OUYFgKIuCq8g-1; Fri, 12 Aug 2022 07:31:33 -0400 X-MC-Unique: TkDx0wuPO0OUYFgKIuCq8g-1 Received: by mail-oi1-f199.google.com with SMTP id q4-20020a0568080ec400b00342b973d2e3so352350oiv.11 for ; Fri, 12 Aug 2022 04:31:33 -0700 (PDT) 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; bh=uGPwSgWp/OfjKIawn6eSMz1+KlPNXJ7/zcrM8Y2Kdl8=; b=HEkeoSk74tawFPUiP1ePX5mQlAZfadmk6Ay1mkp0lYv+V33zB890BvYvdimRI8So78 zxbNI5UZMOpO1IPjP/vIugWakPCUnj9NbyIaZ7TDcaWAFf0aySKgOMgvwyKWt0uJcIPx DNeDloglDiyrj3QQAULTFlAAerf1U0oaV87jEKgPturcQs1fr6AvFJf273TLvvcqbKsI 5ztxql/ano5MRVDS+3dFqgWxzRqyyscwPy+u5T2H7+mmpDMsq+caR1VJ2YxSA7DJK8vO PszLIBsxNBsR9IZnFrAEQGxwcKwLSN1wMxIegjgsZxy87EAc+MByqu1ybV5+WG8IHT6z 44lg== X-Gm-Message-State: ACgBeo0+47r33VBhnst0XZiLliKWSLDlnOrqJT02syHu7Uwg8jh/I6V7 hr5pc6dKCHrJ6tR8xOoC+KsiX62XTpt4K3jQFP/+HNLdSPkU89cY1ErYyRpq8XztoqsbuR+cFpS TYdyoJDRMODD40OtufeE+SwwT85K/mwBHzA== X-Received: by 2002:a05:6830:3905:b0:636:b917:731 with SMTP id br5-20020a056830390500b00636b9170731mr1234298otb.276.1660303892746; Fri, 12 Aug 2022 04:31:32 -0700 (PDT) X-Google-Smtp-Source: AA6agR6qW27tBOvj2FQ0H8qIky69KVlGaYJp2FVT32GkLVMyjgBtH8n5p/OCw9TTEyD8irCS7kIJkRVOGlOtn5MBTRA= X-Received: by 2002:a05:6830:3905:b0:636:b917:731 with SMTP id br5-20020a056830390500b00636b9170731mr1234286otb.276.1660303892429; Fri, 12 Aug 2022 04:31:32 -0700 (PDT) MIME-Version: 1.0 References: <20220812105937.227C413305@imap2.suse-dmz.suse.de> In-Reply-To: <20220812105937.227C413305@imap2.suse-dmz.suse.de> From: Aldy Hernandez Date: Fri, 12 Aug 2022 13:31:21 +0200 Message-ID: Subject: Re: [PATCH] tree-optimization/106593 - fix ICE with backward threading To: Richard Biener , "MacLeod, Andrew" Cc: gcc-patches X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Aug 2022 11:31:38 -0000 On Fri, Aug 12, 2022 at 12:59 PM Richard Biener wrote: > > With the last re-org I failed to make sure to not add SSA names > nor supported by ranger into m_imports which then triggers an > ICE in range_on_path_entry because range_of_expr returns false. I've > noticed that range_on_path_entry does mightly complicated things > that don't make sense to me and the commentary might just be > out of date. For the sake of it I replaced it with range_on_entry > and statistics show we thread _more_ jumps with that, so better > not do magic there. Hang on, hang on. range_on_path_entry was written that way for a reason. Andrew and I had numerous discussions about this. For that matter, my first implementation did exactly what you're proposing, but he had reservations about using range_on_entry, which IIRC he thought should be removed from the (public) API because it had a tendency to blow up lookups. Let's wait for Andrew to chime in on this. If indeed the commentary is out of date, I would much rather use range_on_entry like you propose, but he and I have fought many times about this... over various versions of the path solver :). For now I would return VARYING in range_on_path_entry if range_of_expr returns false. We shouldn't be ICEing when we can gracefully handle things. This gcc_unreachable was there to catch implementation issues during development. I would keep your gimple_range_ssa_p check regardless. No sense doing extra work if we're absolutely sure we won't handle it. Aldy > > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress. > > Will push if that succeeds. > > PR tree-optimization/106593 > * tree-ssa-threadbackward.cc (back_threader::find_paths): > If the imports from the conditional do not satisfy > gimple_range_ssa_p don't try to thread anything. > * gimple-range-path.cc (range_on_path_entry): Just > call range_on_entry. > --- > gcc/gimple-range-path.cc | 33 +-------------------------------- > gcc/tree-ssa-threadbackward.cc | 6 +++++- > 2 files changed, 6 insertions(+), 33 deletions(-) > > diff --git a/gcc/gimple-range-path.cc b/gcc/gimple-range-path.cc > index b6148eb5bd7..a7d277c31b8 100644 > --- a/gcc/gimple-range-path.cc > +++ b/gcc/gimple-range-path.cc > @@ -153,38 +153,7 @@ path_range_query::range_on_path_entry (vrange &r, tree name) > { > gcc_checking_assert (defined_outside_path (name)); > basic_block entry = entry_bb (); > - > - // Prefer to use range_of_expr if we have a statement to look at, > - // since it has better caching than range_on_edge. > - gimple *last = last_stmt (entry); > - if (last) > - { > - if (m_ranger->range_of_expr (r, name, last)) > - return; > - gcc_unreachable (); > - } I > - > - // If we have no statement, look at all the incoming ranges to the > - // block. This can happen when we're querying a block with only an > - // outgoing edge (no statement but the fall through edge), but for > - // which we can determine a range on entry to the block. > - Value_Range tmp (TREE_TYPE (name)); > - bool changed = false; > - r.set_undefined (); > - for (unsigned i = 0; i < EDGE_COUNT (entry->preds); ++i) > - { > - edge e = EDGE_PRED (entry, i); > - if (e->src != ENTRY_BLOCK_PTR_FOR_FN (cfun) > - && m_ranger->range_on_edge (tmp, e, name)) > - { > - r.union_ (tmp); > - changed = true; > - } > - } > - > - // Make sure we don't return UNDEFINED by mistake. > - if (!changed) > - r.set_varying (TREE_TYPE (name)); > + m_ranger->range_on_entry (r, entry, name); > } > > // Return the range of NAME at the end of the path being analyzed. > diff --git a/gcc/tree-ssa-threadbackward.cc b/gcc/tree-ssa-threadbackward.cc > index 0a992213dad..669098e4ec3 100644 > --- a/gcc/tree-ssa-threadbackward.cc > +++ b/gcc/tree-ssa-threadbackward.cc > @@ -525,7 +525,11 @@ back_threader::find_paths (basic_block bb, tree name) > bitmap_clear (m_imports); > ssa_op_iter iter; > FOR_EACH_SSA_TREE_OPERAND (name, stmt, iter, SSA_OP_USE) > - bitmap_set_bit (m_imports, SSA_NAME_VERSION (name)); > + { > + if (!gimple_range_ssa_p (name)) > + return; > + bitmap_set_bit (m_imports, SSA_NAME_VERSION (name)); > + } > > // Interesting is the set of imports we still not have see > // the definition of. So while imports only grow, the > -- > 2.35.3 >