[PATCH] Extend SLPVectorizer for cases where insertelement instructions must be rescheduled

Nadav Rotem nrotem at apple.com
Thu Mar 27 09:42:51 PDT 2014


A few more comments:

  Instruction* x = BN.getInstruction(MyLastIndex);

It should be “Instruction *x” and not ““Instruction* x”.

  while (IE->getParent()==BB) {

Spaces around “==“.


void BoUpSLP::movePrematureInserts(ArrayRef<Value *> VL, InsertElementInst *IE) {
  Instruction *VL0 = cast<Instruction>(VL[0]);
  int MyLastIndex = getLastIndex(VL);
  BasicBlock *BB = cast<Instruction>(VL0)->getParent();
  BlockNumbering &BN = BlocksNumbers[BB];
  DEBUG(dbgs() << "SLP: Moving premature inserts\n");
  Instruction* x = BN.getInstruction(MyLastIndex);

What is X ?   Is it the current insertion place?  If so, it should be called ‘CurrentInsertLoc’. 

    x = IE;
    IE = dyn_cast<InsertElementInst>(IE->user_back());

What happens if IE has more than one user?

  bool tryToVectorizeList(ArrayRef<Value *> VL, BoUpSLP &R, 
                          InsertElementInst *IE = 0, 
                          BoUpSLP::ValueSet *Inserts = 0);

The IE is not a pointer to an Insertion instruction, it is really the first Insert in the build_vector sequence. You should name it “BuildVectorStart” or something to indicate that this is not a single IE instruction. 


static bool findBuildVector(InsertElementInst *IE,
                            SmallVectorImpl<Value *> &Ops,
                            BoUpSLP::ValueSet &Inserts) {

findBuildVector is a really odd function. It returns both the inserts that construct the vector AND the members of the vector? Also, what is IE? what is Ops and what is Inserts?


-Nadav



On Mar 27, 2014, at 9:27 AM, Nadav Rotem <nrotem at apple.com> wrote:

> 
>  Hi Arch,
> 
>  I have a few comments below.
> 
>    /// \brief Move InsertElement instructions with indices preceding
>    /// LastIndex.  \p IE is the root of a chain identified by findBuildVector.
>    void movePrematureInserts(ArrayRef<Value *> VL, InsertElementInst *IE);
> 
>  Please name this method “sinkInstructions”, or something similar that indicates that you are pushing the instructions down. The param IE should be named “location” or something that indicates that this is the destination of the move.
> 
> 
>  void BoUpSLP::buildTree(ArrayRef<Value *> Roots, ValueSet *Rdx,
>                          bool RdxFreeExtract) {
>    assert(!RdxFreeExtract || Rdx);
> 
>  Please add a message to the assertion that explains what went wrong.
> 
> 
>          // Ignore uses that are part of the reduction that do not need extracts.
>          if (RdxFreeExtract)
>            if (std::find(Rdx->begin(), Rdx->end(), UserInst) != Rdx->end())
>              continue;
> 
>  You can && the conditions.
> 
> 
>  void BoUpSLP::movePrematureInserts(ArrayRef<Value *> VL, InsertElementInst *IE) {
>    Instruction *VL0 = cast<Instruction>(VL[0]);
>    int MyLastIndex = getLastIndex(VL);
>    BasicBlock *BB = cast<Instruction>(VL0)->getParent();
>    BlockNumbering &BN = BlocksNumbers[BB];
>    DEBUG(dbgs() << "SLP: Moving premature inserts\n”);
> 
>  period at the end of the comment.
> 
> 
>    Instruction* x = BN.getInstruction(MyLastIndex);
>    while (IE->getParent()==BB) {
>      int UserIndex = BN.getIndex(IE);
>      if (UserIndex >= MyLastIndex) {
>        // Walked past transformed region
> 
> 
>  period at the end of the comment.
> 
>        break;
>      }
> 
>      IE->removeFromParent();
>      IE->insertAfter(x);
> 
> 
>  IE->moveBefore(x)
> 
> 
>      DEBUG(dbgs() << "SLP:    Rescheduled: " << *IE << ".\n");
>      x = IE;
>      IE = dyn_cast<InsertElementInst>(IE->user_back());
>      if (!IE)
>        break;
>    }
>  }
> 
> 
>    /// \brief Try to vectorize a list of operands.
>    /// \returns true if a value was vectorized.
>    bool tryToVectorizeList(ArrayRef<Value *> VL, BoUpSLP &R,
>                            InsertElementInst *IE = 0,
>                            BoUpSLP::ValueSet *Inserts = 0);
> 
> 
>  Please comment the argument names and give them meaningful names.
> 
> 
>      for (; i < NumReducedVals - ReduxWidth + 1; i += ReduxWidth) {
>        ArrayRef<Value *> ValsToReduce(&ReducedVals[i], ReduxWidth);
>        V.buildTree(ValsToReduce, &ReductionOps, true);
> 
>  Please add a comment that explains why you are passing ’true’.
> 
> 
> 
>  Thanks,
>  Nadav
> 
> http://llvm-reviews.chandlerc.com/D3143

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140327/c02e3272/attachment.html>


More information about the llvm-commits mailing list