[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