[PATCH] D22599: GVN-hoist: improve code generation for recursive GEPs

[Daniel Berlin via llvm-commits]

FWIW: In gcc, the regeneration is a pair of mutually recursive functions,
one called "create_expression_by_pieces" (which is the main interface), and
one called "find_or_generate_expression"  (which in turn, calls
create_expression_by_pieces for any operand that is itself a complex value
expression).

Whether you do it similarly, or worklist it, you should have some invariant
that each iteration guarantees or whatever.

I found mutual recursion easier because you can easily guarantee "at the
end of this function we return the thing i need to place in this
expression" for one of the functions, and there you go.


On Wed, Jul 20, 2016 at 3:10 PM, David Majnemer <david.majnemer at gmail.com>
wrote:

> majnemer added inline comments.
>
> ================
> Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:578-603
> @@ +577,28 @@
> +  // Make all operands of the GEP available.
> +  bool makeGepOperandsAvailable(GetElementPtrInst *Gep, BasicBlock
> *HoistPt,
> +                                Instruction *Repl) const {
> +    for (unsigned i = 0, e = Gep->getNumOperands(); i != e; ++i)
> +      if (Instruction *Op = dyn_cast<Instruction>(Gep->getOperand(i))) {
> +
> +        // Check whether the operand is already available.
> +        if (DT->dominates(Op->getParent(), HoistPt))
> +          continue;
> +
> +        // As a GEP can refer to other GEPs, recursively check whether
> all the
> +        // operands of this GEP can be computed at HoistPt.
> +        if (GetElementPtrInst *GepOp = dyn_cast<GetElementPtrInst>(Op))
> +          if (!makeGepOperandsAvailable(GepOp, HoistPt, Gep))
> +            return false;
> +
> +        // Failed to make Op available.
> +        return false;
> +      }
> +
> +    // Copy Gep and replace its uses in Repl with ClonedGep.
> +    Instruction *ClonedGep = Gep->clone();
> +    ClonedGep->insertBefore(HoistPt->getTerminator());
> +    Repl->replaceUsesOfWith(Gep, ClonedGep);
> +
> +    return true;
> +  }
> +
> ----------------
> IIUC, this can mutate the IR while still returning false.
>
> I'd change this to be a worklist algorithm instead of a recursive one.
> Inside the worklist loop, I'd push GEP candidates onto a vector for later
> processing (cloning).
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D22599
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160720/8e74f338/attachment.html>