[PATCH] D23021: GVN-hoist: compute DFS numbers once

[Daniel Berlin via llvm-commits]

Actually, this is very slightly wrong, you will give the wrong order for
the terminator and the instruction before it.
You should increase terminator number by 1, and give your inst the old
terminator number.

On Mon, Aug 1, 2016 at 10:38 AM, Daniel Berlin <dberlin at dberlin.org> wrote:

> If you guarantee you are only inserting at end of block, LGTM
>
> On Mon, Aug 1, 2016 at 10:17 AM, Sebastian Pop <sebpop at gmail.com> wrote:
>
>> sebpop created this revision.
>> sebpop added a reviewer: dberlin.
>> sebpop added a subscriber: llvm-commits.
>>
>> With this patch we compute the DFS numbers of instructions only once and
>> update them during the code generation when an instruction gets hoisted.
>> Passes check and test-suite on x86_64-linux.
>>
>> https://reviews.llvm.org/D23021
>>
>> Files:
>>   llvm/lib/Transforms/Scalar/GVNHoist.cpp
>>
>> Index: llvm/lib/Transforms/Scalar/GVNHoist.cpp
>> ===================================================================
>> --- llvm/lib/Transforms/Scalar/GVNHoist.cpp
>> +++ llvm/lib/Transforms/Scalar/GVNHoist.cpp
>> @@ -72,8 +72,16 @@
>>      //
>>      // assert(A != B);
>>
>> -    unsigned ADFS = DFSNumber.lookup(A);
>> -    unsigned BDFS = DFSNumber.lookup(B);
>> +    const BasicBlock *BA = A->getParent();
>> +    const BasicBlock *BB = B->getParent();
>> +    unsigned ADFS, BDFS;
>> +    if (BA == BB) {
>> +      ADFS = DFSNumber.lookup(A);
>> +      BDFS = DFSNumber.lookup(B);
>> +    } else {
>> +      ADFS = DFSNumber.lookup(BA);
>> +      BDFS = DFSNumber.lookup(BB);
>> +    }
>>      assert (ADFS && BDFS);
>>      return ADFS < BDFS;
>>    }
>> @@ -195,15 +203,17 @@
>>      bool Res = false;
>>      MemorySSA M(F, AA, DT);
>>      MSSA = &M;
>> +    // Perform DFS Numbering of instructions.
>> +    unsigned BBI = 0;
>> +    for (const BasicBlock *BB : depth_first(&F.getEntryBlock())) {
>> +      DFSNumber[BB] = ++BBI;
>> +      unsigned I = 0;
>> +      for (auto &Inst: *BB)
>> +        DFSNumber[&Inst] = ++I;
>> +    }
>>
>>      // FIXME: use lazy evaluation of VN to avoid the fix-point
>> computation.
>>      while (1) {
>> -      // Perform DFS Numbering of instructions.
>> -      unsigned I = 0;
>> -      for (const BasicBlock *BB : depth_first(&F.getEntryBlock()))
>> -        for (auto &Inst: *BB)
>> -          DFSNumber.insert({&Inst, ++I});
>> -
>>        auto HoistStat = hoistExpressions(F);
>>        if (HoistStat.first + HoistStat.second == 0)
>>          return Res;
>> @@ -215,9 +225,6 @@
>>          VN.clear();
>>
>>        Res = true;
>> -
>> -      // DFS numbers change when instructions are hoisted: clear and
>> recompute.
>> -      DFSNumber.clear();
>>      }
>>
>>      return Res;
>> @@ -741,7 +748,10 @@
>>            continue;
>>
>>          // Move the instruction at the end of HoistPt.
>> -        Repl->moveBefore(HoistPt->getTerminator());
>> +        Instruction *Last = HoistPt->getTerminator();
>> +        Repl->moveBefore(Last);
>> +
>> +        DFSNumber[Repl] = DFSNumber[Last]++;
>>        }
>>
>>        MemoryAccess *NewMemAcc = nullptr;
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160801/9a3c3ffb/attachment.html>