[PATCH] D21041: [GVN] PRE can't hoist loads across calls in general.

[Daniel Berlin via llvm-commits]

On Mon, Jun 6, 2016 at 4:06 PM, Eli Friedman <eli.friedman at gmail.com> wrote:

> eli.friedman added a comment.
>
> It probably doesn't make sense to turn every `call` instruction into an
> `invoke` or equivalent; that would cause the size of the IR to explode,


I'm going to disagree.  GCC has EH/abnormal "fake" edges to handle these
cases, and works much better than LLVM in this regard, and has no such
explosion problem on normal C++ code. In fact, it's going to be faster.

If we have a modeling problem, we should fix the modeling problem.
You can abstract this to the block level.  The block has an EH successor,
That just means in some fashion, the block may go to this EH edge.
It does not require you terminate the block at every mayThrow instruction.



> and make the IR more difficult to use for passes which don't actually care
> about "trivial" edges (including most of GVN itself).
>

So far this has been added to three passes, each in an N^2 way.  It is not
the local hoisting that is concerning, it is that every path must have
every instruction checked just to get the single bit that there is a
possible EH successor for a given block.

Anything that wants to hoist or sink has to care, and that's a lot of
passes, despite your claim.



> It might be possible to add some sort of sub-block abstraction over basic
> blocks... for example:
>
>   block:
>     call void @a() ; sub-block 0
>     %y = add i32 %x, 1 ; sub-block 1
>     call void @b() ; sub-block 1
>     ret i32 %y ; sub-block 2
>
> Then we provide some sort of sub-block tree that includes edges to trivial
> unwind blocks.
>
This is not needed, what is needed is to correctly represent that there is
possible abnormal control flow. If something wants to go trying to figure
out what instructions generated it, they could, but the local computation
is already cheap, it's the fact that you have to check every instruction on
every path.


The pass you changed is trying to figure out whether it can hoist into
predecessors.

With EH edges,

    // If any of these blocks has more than one successor (i.e. if the edge
we
    // just traversed was critical), then there are other paths through this
    // block along which the load may not be anticipated.  Hoisting the load
    // above this block would be adding the load to execution paths along
    // which it was not previously executed.
    if (TmpBB->getTerminator()->getNumSuccessors() != 1)
     return false;

Will simply fail and we will go about our way.

Standard dominance checking will also take care of making sure smarter
algorithms don't try to select these things as hoist points.

Trying to figure out if we can move it "just one more instruction" is
pretty much a pointless microoptimization :)



>
> Assuming we have such a thing, the PRE algorithm can iterate over it to
> behave correctly.  (I'm assuming MemoryDependenceAnalysis itself wouldn't
> use it because of the overhead involved.)
>
> The question, of course, is how exactly you would implement this.  If you
> try to compute it on the fly, it boils down to exactly the same loop I've
> already written.  If you try to maintain it as a side-table, you now have a
> gigantic hashtable containing every instruction in the function, and
> modifying the IR requires special sub-block-aware methods to insert,
> remove, or move instructions.  Attaching it to the IR itself adds an
> overhead of at least one pointer per Instruction, plus extra overhead for
> passes which don't care.


I don't think any of these are necessary :)





>


>
> http://reviews.llvm.org/D21041
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160606/3e28c08b/attachment-0001.html>