From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x62c.google.com (mail-ej1-x62c.google.com [IPv6:2a00:1450:4864:20::62c]) by sourceware.org (Postfix) with ESMTPS id 659F43857C49 for ; Tue, 2 Aug 2022 09:11:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 659F43857C49 Received: by mail-ej1-x62c.google.com with SMTP id m4so3024458ejr.3 for ; Tue, 02 Aug 2022 02:11:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=LNBTAD/k6zSDApQtC7FVA4TQKrwDHuS4vh/Ug4HlNzI=; b=s9+6U/DASxpVtC0UAFVIAfEW64DprSihSCBjuMeUoOlMTPapoYJHSpGRL4gz6gh+x1 XfQSw/0n8ls+jGoRAL/UuAV8PUeU96ONKmVjB/q21+hUj1xbuR1zJvAXx+vZAo69Dj7P I9dsguf4cILB2qPf6SNe3N6GsNKchOI0xnXIrpJk7pGnTQzJ/mez8CGmq6rRyNbPC6CK h5GDZid9EJWFmHsYld+b5Y5Tf2cztKnhRXL9e2UJlGeZJMM4tPjolc48W/jJCpg7NzqN 8zVvtq/pDfpobc9JX7lIjOQnJNm3OFSdTSOcFG45Uy+r6CS/z2xB7X3ewHkTi4rqfR1i +YRg== X-Gm-Message-State: AJIora/tUmrJLzbuI3e3T5xxamDgtIwpVYtxeYAHioiPXLST55Ma13yz zbJBjOC1OE0fuk5melpPet4EpbvM328iBiOOePs= X-Google-Smtp-Source: AGRyM1sQE9LBlZTKauA1U3kZIcW3XbMF/wVE2NT9UT07wNyzze8Ypsih69Z18ILhBiCtlTKdAZTqsQlqRtsw/3ajrV4= X-Received: by 2002:a17:907:608d:b0:72f:191b:7625 with SMTP id ht13-20020a170907608d00b0072f191b7625mr15891979ejc.754.1659431487469; Tue, 02 Aug 2022 02:11:27 -0700 (PDT) MIME-Version: 1.0 References: <29n37062-n3s3-71o9-8411-r851o15ss72@fhfr.qr> In-Reply-To: From: Richard Biener Date: Tue, 2 Aug 2022 11:11:14 +0200 Message-ID: Subject: Re: [PATCH 2/2]middle-end: Support recognition of three-way max/min. To: Tamar Christina Cc: Richard Biener , "jakub@redhat.com" , nd , "gcc-patches@gcc.gnu.org" Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-2.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, 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 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: Tue, 02 Aug 2022 09:11:31 -0000 On Tue, Aug 2, 2022 at 10:33 AM Tamar Christina via Gcc-patches wrote: > > > > > > When this function replaces the edge it doesn't seem to update the > > > > dominators. > > > > > Since It's replacing the middle BB we then end up with an error > > > > > > > > > > gcc/testsuite/gcc.dg/tree-ssa/minmax-14.c:17:1: error: dominator > > > > > of 5 should be 4, not 2 > > > > > > > > > > during early verify. So instead, I replace the BB but defer its > > > > > deletion until cleanup which removes it and updates the dominators. > > > > > > > > Hmm, for a diamond shouldn't you replace > > > > > > > > if (EDGE_SUCC (cond_block, 0)->dest == bb) > > > > edge_to_remove = EDGE_SUCC (cond_block, 1); > > > > else > > > > edge_to_remove = EDGE_SUCC (cond_block, 0); > > > > > > > > with > > > > > > > > if (EDGE_SUCC (cond_block, 0)->dest == bb) > > > > edge_to_remove = EDGE_SUCC (cond_block, 1); > > > > else if (EDGE_SUCC (cond_block, 1)->dest == bb) > > > > edge_to_remove = EDGE_SUCC (cond_block, 0); > > > > > > > > thus, the code expects to be left with a fallthru to the PHI block > > > > which is expected to have the immediate dominator being cond_block > > > > but with a diamond there's a (possibly empty) block inbetween and > > > > dominators are wrong. > > > > > > Agreed, but the (EDGE_SUCC (cond_block, 1)->dest == bb) doesn't seem > > > like the Right one since for a diamond there will be a block in > > > between the two. Did you perhaps mean EDGE_SUCC (EDGE_SUCC > > > (cond_block, 1)->dest, 0)->dest == bb? i.e. that that destination across the > > diamond be bb, and then you remove the middle block? > > > > Hmm, I think my condition was correct - the code tries to remove the edge to > > the middle-block and checks the remaining edge falls through to the merge > > block. With a true diamond there is no fallthru to the merge block to keep so > > we better don't remove any edge? > > > > > For the minmax diamond we want both edges removed, since all the code > > > in the middle BBs are now dead. But this is probably not true in the general > > sense. > > Ah! Sorry I was firing a few cylinders short, I get what you mean now: > > @@ -425,8 +439,19 @@ replace_phi_edge_with_variable (basic_block cond_block, > edge edge_to_remove; > if (EDGE_SUCC (cond_block, 0)->dest == bb) > edge_to_remove = EDGE_SUCC (cond_block, 1); > - else > + else if (EDGE_SUCC (cond_block, 1)->dest == bb) > edge_to_remove = EDGE_SUCC (cond_block, 0); > + else > + { > + /* If neither edge from the conditional is the final bb > + then we must have a diamond block, in which case > + the true edge was changed by SET_USE above and we must > + mark the other edge as the false edge. */ > + gcond *cond = as_a (last_stmt (cond_block)); > + gimple_cond_make_false (cond); > + return; > + } > + Note there is already if (EDGE_COUNT (edge_to_remove->dest->preds) == 1) { ... } else { /* If there are other edges into the middle block make CFG cleanup deal with the edge removal to avoid updating dominators here in a non-trivial way. */ gcond *cond = as_a (last_stmt (cond_block)); if (edge_to_remove->flags & EDGE_TRUE_VALUE) gimple_cond_make_false (cond); else gimple_cond_make_true (cond); } I'm not sure how you can say 'e' is always the true edge? May I suggest to amend the first condition with edge_to_remove && (and initialize that to NULL) and use e->flags instead of edge_to_remove in the else, of course also inverting the logic since we're keeping 'e'? > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok with this Change? > > Thanks, > Tamar