On Thu, Jul 9, 2026 at 2:07 PM Jeffrey Law <[email protected]> wrote: > > > > On 7/9/2026 12:21 PM, Andrea Pinski wrote: > > On Thu, Jul 9, 2026 at 8:23 AM Jeffrey Law <[email protected]> > > wrote: > >> > >> > >> On 7/8/2026 2:19 AM, Andrew Pinski wrote: > >>> Since match and simplify can add unused statements to the sequence > >>> in some cases, we should remove them before call phiopt_early_allow. > >>> This is needed for 2 future patches. One is to allowing of comparisons, > >>> optionally with a cast and optional with a negative expression. > >>> The other is about supporting a way to handling > >>> cond_removal_in_builtin_zero_pattern in match and also adding a new match > >>> pattern to fix PR 126035 (ctz split back into one). > >>> > >>> Bootstrapped and tested on x86_64-linux-gnu. > >>> > >>> gcc/ChangeLog: > >>> > >>> * tree-ssa-phiopt.cc (remove_unused_stmts): New function. > >>> (gimple_simplify_phiopt): Call remove_unused_stmts before > >>> phiopt_early_allow. > >> I'm torn. Not particularly happy with what appears to be a mini DCE > >> pass on the sequence. Presumably the motivation is to clean things up > >> in a way that makes phiopt_early_allow more likely to return true by > >> removing statements that are going to be zapped by DCE later and thus > >> shouldn't participate in phiopt_early_allow? > > Yes. While working on a different patch I noticed this. Let me find > > the testcase again. > > > > The testcase is: > > ``` > > bool f1(bool a, bool b) > > { > > bool c = !a; > > bool d = c & b; > > if (d) return 0; > > bool e = c | b; > > return e; > > } > > ``` > > > > This will also be rejected by the current version of > > phiopt_early_allow. But we (Kyrylo and I) are looking to change that. > > With the above patch what we get back is: > > ``` > > phiopt match-simplify back: > > _12 = ~b_11(D); > > _7 = a_9(D) | _12; > > _6 = a_9(D) ^ b_11(D); > > _5 = a_9(D) == b_11(D); > > result: _5 > > rejected because early > > ``` > > But this change we get: > > ``` > > phiopt match-simplify back: > > _5 = a_9(D) == b_11(D); > > result: _5 > > rejected because early > > ``` > > which should then be accepted with the other change we are working on. > > > > This was just one testcase I noticed the issue but I have seen in others. > > The other reason to remove the dead statements from the sequence is > > dealing with my next patch (which is not related to early). Which > > extends phiopt match connection to handle (semi) arbitrary middle bbs > > which allows the removal of cond_removal_in_builtin_zero_pattern and > > allow for some of the patches that Daniel has been working on and also > > allow for matching things like the 2 halved version of ctz into one > > ctz with just a match pattern rather than some manual matching code. > > Basically removing the dead statements makes sure we are no longer > > referencing the middle-bb in some/many cases. > Understood. And presumably the dead statements are unavoidable when > we're going through the simplification steps. I can see why it's > appealing even though I don't like what we have to do to get there. We > may be in a limited-enough environment where a trivial mini-DCE > implementation works sensibly -- things like volatiles, calls, loops and > such shouldn't be a concern here.
> > Presumably the ggc_free is meant to recycle the relevant nodes more > quickly since this is expected to happen reasonably often? Yes. It is similar to what is done in gimple_seq_discard already. > > The LLM review was concerned about the unchecked call to > gimple_get_lhs. But in this context I think every statement we generate > is going to have an LHS. SO that's probably a non-issue. Do we have any > scenario where the LHS isn't going to be an SSA_NAME? I think we > largely filter out memory references to avoid vop updates and such. Note gimple_match_op should never have any memory references in it. (if it does then it is a bug; I fixed one back during GCC 15; r15-3052-gc7b76a076cb2c6) So in this case, what is returned from match always goes though maybe_push_res_to_seq to push things on this sequence that we are going to do a mini-dce on. maybe_push_res_to_seq will always produce either a normal gimple_assign with no load/stores or a call which is always const (it specifically rejects non-const internal/builtin functions; non builtin/internal calls can't be produced). It also rejects ssa names which have abnormal too. So there will always be a LHS. > > I'm inclined to conditionally ACK on the assumption the LLM issues with > gimple_get_lhs and the assumption it always returns an SSA_NAME are > non-issues in this context, but I'd consider this somewhat > controversial, so let's give folks until COB Monday to object to the > basic idea. > > jeff
