[llvm] r228024 - Allow PRE to insert no-cost phi nodes
Jiangning Liu
liujiangning1 at gmail.com
Wed Feb 25 02:04:03 PST 2015
Hi,
Thanks a lot for your explanations! I just came back from Chinese New Year,
and I will have a look around this.
Thanks,
-Jiangning
2015-02-19 4:56 GMT+08:00 Daniel Berlin <dberlin at dberlin.org>:
> Apologies.
> Due to bad filtering, this wasn't ending up in my inbox, just in my
> commits email folder, and i didn't notice it until Chandler mentioned it to
> me . I've fixed this so it shouldn't happen again.
>
>
> The short version is: There is no way to fix this in PRE. The change
> simply eliminates a redundant instruction. It performs no additional
> instruction insertion, etc. It is literally doing nothing but transforming
>
> if (c) {
> e = a +b
> } else {
> f = a + b
> }
> g = a+ b
>
> into
> if (c) {
> e = a +b
> } else {
> f = a + b
> }
> g = phi(e, f)
>
> So it's not even *adding* code (and in fact, it makes the live ranges of a
> and b shorter)
>
> The results for the output should use less registers, not more.
>
> Any normal PRE pass (including our load PRE) will do this, actually. It
> was just an oversight that the existing scalar one had a wrong conditional.
>
> My wild guess is bad register allocation interaction. But if that's the
> case, we should fix that there, because any improvements we make to GVN/PRE
> will exacerbate issues there.
>
> If there is any way to narrow this down a bit, i'd be happy to take a look
> what is going on (though i don't have easy access to the hardware)
>
> On Mon Feb 16 2015 at 6:17:25 PM Jiangning Liu <liujiangning1 at gmail.com>
> wrote:
>
>> Ping again...
>>
>> 2015-02-15 9:26 GMT+08:00 Jiangning Liu <liujiangning1 at gmail.com>:
>>
>>> Hi Daniel,
>>>
>>> Ping...
>>>
>>> Thanks,
>>> -Jiangning
>>>
>>>
>>> 2015-02-10 14:34 GMT+08:00 Jiangning Liu <liujiangning1 at gmail.com>:
>>>
>>>> Hi Daniel,
>>>>
>>>> This commit caused ~5% performance regression for spec2006.401.bzip2 on
>>>> aarch64.
>>>>
>>>> Don't know the root cause yet, but looking at the binary of
>>>> decompress.o, it seems the code change is significant.
>>>>
>>>> Have you noticed the same issue?
>>>>
>>>> Thanks,
>>>> -Jiangning
>>>>
>>>>
>>>>
>>>> 2015-02-04 4:37 GMT+08:00 Daniel Berlin <dberlin at dberlin.org>:
>>>>
>>>>> Author: dannyb
>>>>> Date: Tue Feb 3 14:37:08 2015
>>>>> New Revision: 228024
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=228024&view=rev
>>>>> Log:
>>>>> Allow PRE to insert no-cost phi nodes
>>>>>
>>>>> Added:
>>>>> llvm/trunk/test/Transforms/GVN/pre-no-cost-phi.ll
>>>>> Modified:
>>>>> llvm/trunk/lib/Transforms/Scalar/GVN.cpp
>>>>>
>>>>> Modified: llvm/trunk/lib/Transforms/Scalar/GVN.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVN.cpp?rev=228024&r1=228023&r2=228024&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/Transforms/Scalar/GVN.cpp (original)
>>>>> +++ llvm/trunk/lib/Transforms/Scalar/GVN.cpp Tue Feb 3 14:37:08 2015
>>>>> @@ -711,6 +711,8 @@ namespace {
>>>>> bool iterateOnFunction(Function &F);
>>>>> bool performPRE(Function &F);
>>>>> bool performScalarPRE(Instruction *I);
>>>>> + bool performScalarPREInsertion(Instruction *Instr, BasicBlock
>>>>> *Pred,
>>>>> + unsigned int ValNo);
>>>>> Value *findLeader(const BasicBlock *BB, uint32_t num);
>>>>> void cleanupGlobalSets();
>>>>> void verifyRemoved(const Instruction *I) const;
>>>>> @@ -2454,6 +2456,43 @@ bool GVN::processBlock(BasicBlock *BB) {
>>>>> return ChangedFunction;
>>>>> }
>>>>>
>>>>> +// Instantiate an expression in a predecessor that lacked it.
>>>>> +bool GVN::performScalarPREInsertion(Instruction *Instr, BasicBlock
>>>>> *Pred,
>>>>> + unsigned int ValNo) {
>>>>> + // Because we are going top-down through the block, all value
>>>>> numbers
>>>>> + // will be available in the predecessor by the time we need them.
>>>>> Any
>>>>> + // that weren't originally present will have been instantiated
>>>>> earlier
>>>>> + // in this loop.
>>>>> + bool success = true;
>>>>> + for (unsigned i = 0, e = Instr->getNumOperands(); i != e; ++i) {
>>>>> + Value *Op = Instr->getOperand(i);
>>>>> + if (isa<Argument>(Op) || isa<Constant>(Op) ||
>>>>> isa<GlobalValue>(Op))
>>>>> + continue;
>>>>> +
>>>>> + if (Value *V = findLeader(Pred, VN.lookup(Op))) {
>>>>> + Instr->setOperand(i, V);
>>>>> + } else {
>>>>> + success = false;
>>>>> + break;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + // Fail out if we encounter an operand that is not available in
>>>>> + // the PRE predecessor. This is typically because of loads which
>>>>> + // are not value numbered precisely.
>>>>> + if (!success)
>>>>> + return false;
>>>>> +
>>>>> + Instr->insertBefore(Pred->getTerminator());
>>>>> + Instr->setName(Instr->getName() + ".pre");
>>>>> + Instr->setDebugLoc(Instr->getDebugLoc());
>>>>> + VN.add(Instr, ValNo);
>>>>> +
>>>>> + // Update the availability map to include the new instruction.
>>>>> + addToLeaderTable(ValNo, Instr, Pred);
>>>>> + return true;
>>>>> +}
>>>>> +
>>>>> bool GVN::performScalarPRE(Instruction *CurInst) {
>>>>> SmallVector<std::pair<Value*, BasicBlock*>, 8> predMap;
>>>>>
>>>>> @@ -2520,60 +2559,43 @@ bool GVN::performScalarPRE(Instruction *
>>>>>
>>>>> // Don't do PRE when it might increase code size, i.e. when
>>>>> // we would need to insert instructions in more than one pred.
>>>>> - if (NumWithout != 1 || NumWith == 0)
>>>>> - return false;
>>>>> -
>>>>> - // Don't do PRE across indirect branch.
>>>>> - if (isa<IndirectBrInst>(PREPred->getTerminator()))
>>>>> + if (NumWithout > 1 || NumWith == 0)
>>>>> return false;
>>>>>
>>>>> - // We can't do PRE safely on a critical edge, so instead we schedule
>>>>> - // the edge to be split and perform the PRE the next time we iterate
>>>>> - // on the function.
>>>>> - unsigned SuccNum = GetSuccessorNumber(PREPred, CurrentBlock);
>>>>> - if (isCriticalEdge(PREPred->getTerminator(), SuccNum)) {
>>>>> - toSplit.push_back(std::make_pair(PREPred->getTerminator(),
>>>>> SuccNum));
>>>>> - return false;
>>>>> - }
>>>>> + // We may have a case where all predecessors have the instruction,
>>>>> + // and we just need to insert a phi node. Otherwise, perform
>>>>> + // insertion.
>>>>> + Instruction *PREInstr = nullptr;
>>>>>
>>>>> - // Instantiate the expression in the predecessor that lacked it.
>>>>> - // Because we are going top-down through the block, all value
>>>>> numbers
>>>>> - // will be available in the predecessor by the time we need them.
>>>>> Any
>>>>> - // that weren't originally present will have been instantiated
>>>>> earlier
>>>>> - // in this loop.
>>>>> - Instruction *PREInstr = CurInst->clone();
>>>>> - bool success = true;
>>>>> - for (unsigned i = 0, e = CurInst->getNumOperands(); i != e; ++i) {
>>>>> - Value *Op = PREInstr->getOperand(i);
>>>>> - if (isa<Argument>(Op) || isa<Constant>(Op) ||
>>>>> isa<GlobalValue>(Op))
>>>>> - continue;
>>>>> + if (NumWithout != 0) {
>>>>> + // Don't do PRE across indirect branch.
>>>>> + if (isa<IndirectBrInst>(PREPred->getTerminator()))
>>>>> + return false;
>>>>>
>>>>> - if (Value *V = findLeader(PREPred, VN.lookup(Op))) {
>>>>> - PREInstr->setOperand(i, V);
>>>>> - } else {
>>>>> - success = false;
>>>>> - break;
>>>>> + // We can't do PRE safely on a critical edge, so instead we
>>>>> schedule
>>>>> + // the edge to be split and perform the PRE the next time we
>>>>> iterate
>>>>> + // on the function.
>>>>> + unsigned SuccNum = GetSuccessorNumber(PREPred, CurrentBlock);
>>>>> + if (isCriticalEdge(PREPred->getTerminator(), SuccNum)) {
>>>>> + toSplit.push_back(std::make_pair(PREPred->getTerminator(),
>>>>> SuccNum));
>>>>> + return false;
>>>>> + }
>>>>> + // We need to insert somewhere, so let's give it a shot
>>>>> + PREInstr = CurInst->clone();
>>>>> + if (!performScalarPREInsertion(PREInstr, PREPred, ValNo)) {
>>>>> + // If we failed insertion, make sure we remove the instruction.
>>>>> + DEBUG(verifyRemoved(PREInstr));
>>>>> + delete PREInstr;
>>>>> + return false;
>>>>> }
>>>>> }
>>>>>
>>>>> - // Fail out if we encounter an operand that is not available in
>>>>> - // the PRE predecessor. This is typically because of loads which
>>>>> - // are not value numbered precisely.
>>>>> - if (!success) {
>>>>> - DEBUG(verifyRemoved(PREInstr));
>>>>> - delete PREInstr;
>>>>> - return false;
>>>>> - }
>>>>> + // Either we should have filled in the PRE instruction, or we should
>>>>> + // not have needed insertions.
>>>>> + assert (PREInstr != nullptr || NumWithout == 0);
>>>>>
>>>>> - PREInstr->insertBefore(PREPred->getTerminator());
>>>>> - PREInstr->setName(CurInst->getName() + ".pre");
>>>>> - PREInstr->setDebugLoc(CurInst->getDebugLoc());
>>>>> - VN.add(PREInstr, ValNo);
>>>>> ++NumGVNPRE;
>>>>>
>>>>> - // Update the availability map to include the new instruction.
>>>>> - addToLeaderTable(ValNo, PREInstr, PREPred);
>>>>> -
>>>>> // Create a PHI to make the value available in this block.
>>>>> PHINode *Phi =
>>>>> PHINode::Create(CurInst->getType(), predMap.size(),
>>>>> @@ -2609,6 +2631,8 @@ bool GVN::performScalarPRE(Instruction *
>>>>> MD->removeInstruction(CurInst);
>>>>> DEBUG(verifyRemoved(CurInst));
>>>>> CurInst->eraseFromParent();
>>>>> + ++NumGVNInstr;
>>>>> +
>>>>> return true;
>>>>> }
>>>>>
>>>>>
>>>>> Added: llvm/trunk/test/Transforms/GVN/pre-no-cost-phi.ll
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/pre-no-cost-phi.ll?rev=228024&view=auto
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/test/Transforms/GVN/pre-no-cost-phi.ll (added)
>>>>> +++ llvm/trunk/test/Transforms/GVN/pre-no-cost-phi.ll Tue Feb 3
>>>>> 14:37:08 2015
>>>>> @@ -0,0 +1,31 @@
>>>>> +; RUN: opt < %s -gvn -S | FileCheck %s
>>>>> +; This testcase tests insertion of no-cost phis. That is,
>>>>> +; when the value is already available in every predecessor,
>>>>> +; and we just need to insert a phi node to merge the available values.
>>>>> +
>>>>> + at c = global i32 0, align 4
>>>>> + at d = global i32 0, align 4
>>>>> +
>>>>> +
>>>>> +define i32 @mai(i32 %foo, i32 %a, i32 %b) {
>>>>> + %1 = icmp ne i32 %foo, 0
>>>>> + br i1 %1, label %bb1, label %bb2
>>>>> +
>>>>> +bb1:
>>>>> + %2 = add nsw i32 %a, %b
>>>>> + store i32 %2, i32* @c, align 4
>>>>> + br label %mergeblock
>>>>> +
>>>>> +bb2:
>>>>> + %3 = add nsw i32 %a, %b
>>>>> + store i32 %3, i32* @d, align 4
>>>>> + br label %mergeblock
>>>>> +
>>>>> +mergeblock:
>>>>> +; CHECK: pre-phi = phi i32 [ %3, %bb2 ], [ %2, %bb1 ]
>>>>> +; CHECK-NEXT: ret i32 %.pre-phi
>>>>> + %4 = add nsw i32 %a, %b
>>>>> + ret i32 %4
>>>>> +}
>>>>> +
>>>>> +
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>
>>>>
>>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150225/ed6c7016/attachment.html>
More information about the llvm-commits
mailing list