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

[Nadav Rotem]

  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