On 2023/3/9 20:02, Richard Biener wrote:
> On Wed, 8 Mar 2023, Xionghu Luo wrote:
> 
>>
>>
>> On 2023/3/7 19:25, Richard Biener wrote:
>>>>> It would be nice to avoid creating blocks / preserving labels we'll
>>>>> immediately remove again.  For that we do need some analysis
>>>>> before creating basic-blocks that determines whether a label is
>>>>> possibly reached by a non-falltru edge.
>>>>>
>>>>
>>>> <bb 2> :
>>>> p = 0;
>>>> switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>
>>>>
>>>> <bb 3> :
>>>> <L0>:           <= prev_stmt
>>>> <D.2748>:       <= stmt
>>>> p = p + 1;
>>>> n = n + -1;
>>>> if (n != 0) goto <D.2748>; else goto <D.2746>;
>>>>
>>>> Check if <L0> is a case label and <D.2748> is a goto target then return
>>>> true
>>>> in stmt_starts_bb_p to start a new basic block?  This would avoid creating
>>>> and
>>>> removing blocks, but cleanup_dead_labels has all bbs setup while
>>>> stmt_starts_bb_p
>>>> does't yet to iterate bbs/labels to establish label_for_bb[] map?
>>
>>> Yes.  I think we'd need something more pragmatic before make_blocks (),
>>> like re-computing TREE_USED of the label decls or computing a bitmap
>>> of targeted labels (targeted by goto, switch or any other means).
>>>
>>> I'll note that doing a cleanup_dead_labels () like optimization before
>>> we create blocks will help keeping LABEL_DECL_UID and thus
>>> label_to_block_map dense.  But it does look like a bit of
>>> an chicken-and-egg problem and the question is how effective the
>>> dead label removal is in practice.
>>
>> Tried to add function compute_target_labels(not sure whether the function
>> name is suitable) in the front of make_blocks_1, now the fortran case doesn't
>> create/removing blocks now, but I still have several questions:
>>
>>   1. I used hash_set<tree> to save the target labels instead of bitmap, as
>> labels
>> are tree type value instead of block index so bitmap is not good for it since
>> we don't have LABEL_DECL_UID now?
> 
> We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of
> hash_set<tree> vs. bitmap is somewhat arbitrary here.  The real cost is
> the extra walk over all stmts.
> 
>>   2. Is the compute_target_labels still only for !optimize?  And if we compute
>> the target labels before create bbs, it is unnessary to guard the first
>> cleanup_dead_labels under !optimize now, because the switch-case-do-while
>> case already create new block for CASE_LABEL already.
> 
> OK.
> 
>>   3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
>> so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
>> labels_eh?
> 
> I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and
> coverage already?
> 
>> PS1: The v3 patch will cause one test case fail:
>>
>> Number of regressions in total: 1
>>> FAIL: gcc.c-torture/compile/limits-caselabels.c   -O0  (test for excess
>>> errors)
>>
>> due to this exausting case has labels from L0 to L100001, they won't be
>> optimized
>> to a simple if-else expression like before...
> 
> Hmm, that's somewhat unexpected.
> 
>>
>> PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail due
>> to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into it
>> so
>> just skip these label...
> 
> Please investigate, we might be missing a corner case here.
> 

I think the *previous fix* for labels “in the middle of block” is *incorrect*, it should
be handled in make_edges_bb when a basic block only has Label in it, just create a
fallthrough edge for it to avoid wrong cfg and unreachable trap generated?


@@ -853,6 +922,12 @@ make_edges_bb (basic_block bb, struct omp_region **pcur_region, int *pomp_index)
    bool fallthru = false;
    int ret = 0;

+  if (!optimize && !last)
+    {
+      make_edge (bb, bb->next_bb, EDGE_FALLTHRU);
+      return 0;
+    }
+
    if (!last)
      return ret;


With the fix, the attached version could pass bootstrap and regression test on x86_64-linux-gnu.