From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 21DCF3858C52 for ; Fri, 12 Aug 2022 11:35:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 21DCF3858C52 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 563A534FAC; Fri, 12 Aug 2022 11:35:53 +0000 (UTC) Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 5036D2C141; Fri, 12 Aug 2022 11:35:53 +0000 (UTC) Date: Fri, 12 Aug 2022 11:35:53 +0000 (UTC) From: Richard Biener To: Aldy Hernandez cc: "MacLeod, Andrew" , gcc-patches Subject: Re: [PATCH] tree-optimization/106593 - fix ICE with backward threading In-Reply-To: Message-ID: References: <20220812105937.227C413305@imap2.suse-dmz.suse.de> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, 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:35:55 -0000 On Fri, 12 Aug 2022, Aldy Hernandez wrote: > 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. OK, I'll push just the gimple_range_ssa_p then since that resolves the PR on its own. I was first misled about the gcc_unreachable and my brain hurt understanding this function ... (also as to why using range_of_expr on a _random_ stmt would be OK). That said, nothing seems to be (publicly) using range_on_entry, so if it shouldn't be used (but it's used privately!) then make it private. Thanks, Richard. > 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 > > > > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)