[PATCH] D20143: New pass: guard widening

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

In http://reviews.llvm.org/D20143#431592, @atrick wrote:

> I did a fairly quick review. Your pass looks pretty great.
>
> I don't like to see unbounded recursion through multiple operands:
>
> +  return all_of(Inst->operands(), [&](Value *Op) {
>  +      return isAvailableAt(Op, Loc);
>  +    });
>
> ...having had to fix many of these problems in LLVM when a
>  pathological case leads to exponential compile time. As a rule, I
>  think you either need a very small limit on the recursion, or a
>  visited set.


For now I'll add a cap on the recursion.  This is something I expect
to revisit once I have this running over Real Code(TM).

> You're evaluating every guard against all dominating guard intrinsics:

> 

> +  for (unsigned i = 0, e = DFSI.getPathLength(); i != e; ++i) {

>  +    const auto &GuardsInCurBB = GuardsInBlock.find(CurBB)->second;

>  +    auto I = GuardsInCurBB.begin();

>  +    auto E = GuardsInCurBB.end();

>  +    for (auto *Candidate : make_range(I, E)) {

> 

> It's a nice simple design, but I wonder how well it scales (with

>  massive inlining). Since you're only matching guards with the same LHS


Honestly, I suspect it won't scale very well.  My priority now is to run some
real world code through this, and get a stronger sense of what
matters; and use that information to help drive improvements here.

> cmp operand, you could use a scoped hashtable instead. That way most

>  guards don't need to be searched. OTOH you would be maintaining a

>  complex side data structure.



http://reviews.llvm.org/D20143