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

[David Majnemer via llvm-commits]

On Wed, Jul 20, 2016 at 6:04 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

> 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.
>

Both of these approaches sound find to me, I'd just prefer avoiding the
creation of unnecessary IR.


>
>
> 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/727eca29/attachment.html>