Re: RFA: Fix PR rtl-optimization/60651

[Joern Rennecke <joern dot rennecke at embecosm dot com>]

On 28 March 2014 10:20, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> However, the first call is for blocks with incoming abnormal edges.
>> If these are empty, the change as I wrote it yesterday is fine, but not
>> when they are non-empty; in that case, we should indeed insert before the
>> first instruction in that block.
>
> OK, so the issue is specific to empty basic blocks and boils down to inserting
> instructions in a FIFO manner into them.

Actually, the issue also applies to abnormal edges where lcm did leave a set -
but these are rare, and my last patch should handle these properly in any event,
by no longer using the NOTE_INSN_BASIC_BLOCK itself unless the block is
empty.

>> This can be archived by finding an insert-before position using NEXT_INSN
>> on the basic block head; this amounts to the very same insertion place
>> as inserting after the basic block head.  Also, we will continue to set no
>> location, and use the same bb, because both add_insn_before and
>> add_insn_after (in contradiction to its block comment) will infer the basic
>> block from the insn given (in the case for add_insn_before, I assume
>> that the basic block doesn't start with a BARRIER - that would be invalid -
>> and that the insn it starts with has a valid BLOCK_FOR_INSN setting the
>> same way the basic block head has.
>
> This looks reasonable, but I think that we need more commentary because it's
> not straightforward to understand, so I would:
>
>   1. explicitly state that we enforce an order on the entities in addition to
> the order on priority, both in the code (for example create a 4th paragraph in
> the comment at the top of the file, before "More details ...") and in the doc
> as you already did, but "ordering" the two orders for the sake of clarity:
> first the order on priority then, for the same priority, the order to the
> entities.

Actually, all the patch provides is a partial order, just as I stated.
Providing the strict order you describe would require adding another
loop nesting to the entity/basic block/seginfo loop, and it wouldn't
really be useful for targets.
To order by entity first, then by priority, could be useful for some targets,
so that they can express a dependency chain of mode switching events
to be computed in a single lcm pass without inflating the mode count
(which determines how often we have to invoke the lcm machinery).
However, that would require having separate buckets for each entity for
each  insert_insn_on_edge point.

For epiphany,  EPIPHANY_MSW_ENTITY_FPU_OMNIBUS (for -O0) and
EPIPHANY_MSW_ENTITY_ROUND_KNOWN (used when optimizing)
depend on EPIPHANY_MSW_ENTITY_AND,  EPIPHANY_MSW_ENTITY_OR and
EPIPHANY_MSW_ENTITY_CONFIG.
The latter three only have two modes, an the former two use the
enum attr_fp_mode values, the first of which is FP_MODE_ROUND_UNKNOWN.
That value does not actually appear as a needed mode for these entities, hence
the partial order is sufficient.

EPIPHANY_MSW_ENTITY_FPU_OMNIBUS also depends on EPIPHANY_MSW_ENTITY_OR.

>   2. add a line in the head comment of new_seginfo saying that INSN may not be
> a NOTE_BASIC_BLOCK, unless BB is empty.
>
>   3. add a comment above the trick in optimize_mode_switching saying that it
> is both required to implement the FIFO insertion and valid because we know
> that the basic block was initially empty.

Done.

> It's not clear to me whether this is a regression or not, so you'll also need
> to run it by the RMs.

I don't think it's a regression.

Attachment: pr60651-0402.diff
Description: Text document