From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x630.google.com (mail-ej1-x630.google.com [IPv6:2a00:1450:4864:20::630]) by sourceware.org (Postfix) with ESMTPS id 72A863858C00 for ; Thu, 14 Jul 2022 06:39:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 72A863858C00 Received: by mail-ej1-x630.google.com with SMTP id j22so1671094ejs.2 for ; Wed, 13 Jul 2022 23:39:02 -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=w2M9ffuIpWg/ZB84LQ+eGDLRXeTSwWNPubr46cXDrhI=; b=F9SggiAc9Cg+4hoNxvqoqLbKdFL1gHp9o1lq5R7liE1dI4dz5rZUXh6+x0pJuHeoFW kuq+SkGZyjQRv+FT+XkIz/iX63KzDuicld0AyRHeAptYMPSX6UbVoD1yGEcrOVNhX4mN SBItbGHdjcgnUkWj3GuZlr/XHMyDARLltSCVhBvweBKaVvoljbKEH1ajDTtnwGqXIJSn YLGhoSeiB2wlynCy6I5fqqsW12jKxS3Jlf+6QXf9bTu+v0RrKTd/Jg0TxRhGHgtxsFsd Q0qykfeW574errqTorABsAdxhumND1pdZl4uHHrhyvk76uJujSZVTMVzovbsFvvZVmpo dTZA== X-Gm-Message-State: AJIora9zinxfVa9aiQOIT+pB77uNNc+7/KPtRjzzI1oY1oqz93xTNJUM +aK6i77tbWK7lLBNgNDOKf8rXhZLHzhHPNCBo3Y= X-Google-Smtp-Source: AGRyM1uPNdXVoiQknnH4HhQ8KNn/OO80YUsnh0PoKGeonHL/zAs+mVZlm1/ZR2YFokmHTKjmeAkkd+KoQ90ELUGuCYk= X-Received: by 2002:a17:907:7209:b0:72b:924b:60a8 with SMTP id dr9-20020a170907720900b0072b924b60a8mr7487130ejc.442.1657780740921; Wed, 13 Jul 2022 23:39:00 -0700 (PDT) MIME-Version: 1.0 References: <20220114182047.6270-3-amonakov@ispras.ru> <84dba16-ab1a-f362-48-8f42ebc14d2b@ispras.ru> In-Reply-To: <84dba16-ab1a-f362-48-8f42ebc14d2b@ispras.ru> From: Richard Biener Date: Thu, 14 Jul 2022 08:38:48 +0200 Message-ID: Subject: Re: [PATCH 2/3] tree-cfg: do not duplicate returns_twice calls To: Alexander Monakov Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, 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: Thu, 14 Jul 2022 06:39:06 -0000 On Wed, Jul 13, 2022 at 4:57 PM Alexander Monakov wrote: > > On Wed, 13 Jul 2022, Richard Biener wrote: > > > > > The thing to check would be incoming abnormal edges in > > > > can_duplicate_block_p, not (only) returns twice functions? > > > > > > Unfortunately not, abnormal edges are also used for computed gotos, which are > > > less magic than returns_twice edges and should not block tracer I think. > > > > I think computed gotos should use regular edges, only non-local goto should > > use abnormals... > > Yeah, afaict it's not documented what "abnormal" is supposed to mean :/ > > > I suppose asm goto also uses abnormal edges? > > Heh, no, asm goto appears to use normal edges, but there's an old gap in > their specification: can you use them like computed gotos, i.e. can asm-goto > jump to a computed target? Or must they be similar to plain gotos where the > jump label is redirectable (because it's substitutable in the asm template)? > > If you take a restrictive interpretation (asm goto may not jump to a computed > label) then using regular edges looks fine. > > > Btw, I don't see how they in general are "less magic". Sure, we have an > > explicit receiver (the destination label), but we can only do edge inserts > > if we have a single computed goto edge into a block (we can "move" the > > label to the block created when splitting the edge). > > Sure, they are a bit magic, but returns_twice edges are even more magic: their > destination looks tied to a label in the IR, but in reality their destination > is inside a call that returns twice (hence GCC must be careful not to insert > anything between the label and the call, like in patch 1/3). Indeed. Guess that's what __builtin_setjmp[_receiver] for SJLJ_EH got "right". When copying a block we do not copy labels so any "jumps" remain to the original block and thus we are indeed able to isolate normal control flow. Given that returns_twice functions _do_ seem to be special, and also special as to how we handle other abnormal receivers in duplicate_block. So it might indeed make sense to special-case them in can_duplicate_block_p ... (sorry for going back-and-forth ...) Note that I think this detail of duplicate_block (the function) and the hook needs to be better documented (the semantics on incoming edges, not duplicating labels used for incoming control flow). Can you see as to how to adjust the RTL side for this? It looks like at least some places set a REG_SETJMP note on call_insns (emit_call_1), I wonder if in rtl_verify_flow_info[_1] (or its callees) we can check that such calls come first ... they might not since IIRC we do _not_ preserve abnormal edges when expanding RTL (there's some existing bug about this and how this breaks some setjmp tests) (but we try to recompute them?). Sorry about the back-and-forth again ... your original patch looks OK for the GIMPLE side but can you amend the cfghooks.{cc,h} documentation to summarize our findings and the desired semantics of duplicate_block in this respect? Thanks, Richard. > > > This implies patch 1/3 [1] unnecessary blocks sinking to computed goto targets. > > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588498.html > > > > > > How would you like to proceed here? Is my initial patch ok? > > > > Hmm, so for returns twice calls duplicate_block correctly copies the call > > and redirects the provided incoming edge to it. The API does not > > handle adding any further incoming edges - the caller would be responsible > > for this. So I still somewhat fail to see the point here. If tracer does not > > handle extra incoming edges properly then we need to fix tracer? > > I think abnormal edges corresponding to computed gotos are fine: we are > attempting to create a chain of blocks with no incoming edges in the middle, > right? Destinations of computed gotos remain at labels of original blocks. > > Agreed about correcting this in the tracer. > > > This also includes non-local goto (we seem to copy non-local labels just > > fine - wasn't there a bugreport about this!?). > > Sorry, no idea about this. > > > So I think can_duplicate_block_p is the wrong place to fix (the RTL side > > would need a similar fix anyhow?) > > Right. I'm happy to leave both RTL and GIMPLE can_duplicate_block_p as is, > and instead constrain just the tracer. Alternative patch below: > > * tracer.cc (analyze_bb): Disallow duplication of returns_twice calls. > > diff --git a/gcc/tracer.cc b/gcc/tracer.cc > index 64517846d..422e2b6a7 100644 > --- a/gcc/tracer.cc > +++ b/gcc/tracer.cc > @@ -132,14 +132,19 @@ analyze_bb (basic_block bb, int *count) > gimple *stmt; > int n = 0; > > + bool can_dup = can_duplicate_block_p (CONST_CAST_BB (bb)); > + > for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) > { > stmt = gsi_stmt (gsi); > n += estimate_num_insns (stmt, &eni_size_weights); > + if (can_dup && cfun->calls_setjmp && gimple_code (stmt) == GIMPLE_CALL > + && gimple_call_flags (stmt) & ECF_RETURNS_TWICE) > + can_dup = false; > } > *count = n; > > - cache_can_duplicate_bb_p (bb, can_duplicate_block_p (CONST_CAST_BB (bb))); > + cache_can_duplicate_bb_p (bb, can_dup); > } > > /* Return true if E1 is more frequent than E2. */ >