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). > > 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. */
