On 10/26/2017 02:55 PM, Eric Botcazou wrote:
> The attached Ada testcase triggers an ICE at -O3 when compiled for x86:
> 
> eric@polaris:~/build/gcc/native> gcc/xgcc -Bprev-gcc -S opt68.adb -O3 -m32
> opt68.adb: In function 'Opt68.Copy':
> opt68.adb:51:6: error: multiple hot/cold transitions found (bb 34)
> opt68.adb:51:6: error: multiple hot/cold transitions found (bb 64)
> +===========================GNAT BUG DETECTED==============================+
> | 8.0.0 20171026 (experimental) [trunk revision 254096] (x86_64-suse-linux) 
> GCC error:|
> | verify_flow_info failed                                                  |
> 
> It's the RTL basic block reordering pass confusing itself, more precisely:
> 
>         /* If the best destination has multiple predecessors, and can be
>            duplicated cheaper than a jump, don't allow it to be added
>            to a trace.  We'll duplicate it when connecting traces.  */
>         if (best_edge && EDGE_COUNT (best_edge->dest->preds) >= 2
>             && copy_bb_p (best_edge->dest, 0))
>           best_edge = NULL;
> 
> Now, if you're in the first round (reordering of the hot paritition) and the 
> basic block has 2 predecessors, a regular one (corresponding to best_edge) 
> and 
> another coming from the cold partition (i.e. EDGE_CROSSING), the duplication 
> will redirect best_edge and leave the block with only one predecessor coming 
> from the cold partition, which means that the block will eventually become 
> cold at the end of the pass if fixup_partitions is invoked.  But that's too 
> late since
> 
>   /* Signal that rtl_verify_flow_info_1 can now verify that there
>      is at most one switch between hot/cold sections.  */
>   crtl->bb_reorder_complete = true;
> 
> So we would need to re-run the pass in order to move the block from the hot 
> partition to the cold partition (re-running only a subpass wouldn't work 
> since 
> the above decision is made by find_traces but the duplication is triggered by 
> connect_traces), which seems overkill.
> 
> Therefore the attached patch detects this problematic case and doesn't reset 
> best_edge upon finding it.  This means that the destination will be added to 
> the current trace in the hot partition, which looks OK since all its other 
> predecessors are in the cold partition.  It also contains a fix for an off-by-
> one bug in the dump file and removes a redundant check from better_edge_p 
> (the 
> same check on the BB_PARTITION of the blocks is done in find_traces_1_round).
> 
> Bootstrapped/regtested on x86_64-suse-linux.  Any objection?
> 
> 
> 2017-10-26  Eric Botcazou  <ebotca...@adacore.com>
> 
>       * bb-reorder.c (find_traces_1_round): Fix off-by-one index.
>       Move around comment.  Do not reset best_edge for a copiable
>       destination if the copy would cause a partition change.
>       (better_edge_p): Remove redundant check.
> 
> 
> 2017-10-26  Eric Botcazou  <ebotca...@adacore.com>
> 
>       * gnat.dg/opt68.ad[sb]: New test.
OK.
jeff

Reply via email to