[SLP] Remove LHS and RHS from OperationData.

[craig.topper]

Summary

These were only really used for 2 things. One was to check if the operand matches the phi if it exists. The other was for the createOp method to build the reduction.

For the first case we still have the operation we just need to know how to index its operands. So I've modified getLHS/getRHS to just use the opcode/kind to know how to find the right operands on an instruction that is now passed in.

For the other case we had to create an OperationData object to set the LHS/RHS values and copy the opcode/kind from another object. We would then just call createOp on that temporary object. Instead I've made LHS/RHS arguments to createOp and removed all these temporary objects.

Ultimately, I'm just trying to familiarize myself with this code so I can figure how fast math flags should work. Thought I'd try make some improvements along the way.

[craig.topper]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

6484

I'm not entirely sure what this assert was trying to check before.

As far as I can tell there are 3 states OperationData could have

• Kind is RK_None and Opcode is 0 and LHS/RHS wre.
• Kind is RK_None and Opcode is non-zero and LHS/RHS are null
• Kind is not RK_None and Opcode is non-zero and LHS/RHS are non-null

So I think this was previously saying that if the Kinds are equal then the LHS/RHS in both should be null or non-null. Maybe I should have changed it to (Kind != OD.Kind) || ((Opcode != 0) == (OD.Opcode != 0))?

6499

I remove this pointer comparison. I didn't see any reason checking the fields wasn't ok this and OD being the same object.

[ABataev]

LG

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

6484

I think this assert is good in its current form.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

enum ReductionKind {
     RK_UMax,       /// Unsigned maximum reduction data.
   };
 
   /// Contains info about operation, like its opcode, left and right operands.
   class OperationData {
     /// Opcode of the instruction.
     unsigned Opcode = 0;
 
-    /// Left operand of the reduction operation.
-    Value *LHS = nullptr;
-
-    /// Right operand of the reduction operation.
-    Value *RHS = nullptr;
-
     /// Kind of the reduction operation.
     ReductionKind Kind = RK_None;
 
     /// Checks if the reduction operation can be vectorized.
     bool isVectorizable() const {
-      return LHS && RHS &&
-             // We currently only support add/mul/logical && min/max reductions.
-             ((Kind == RK_Arithmetic &&
+      return ((Kind == RK_Arithmetic &&
                (Opcode == Instruction::Add || Opcode == Instruction::FAdd ||
                 Opcode == Instruction::Mul || Opcode == Instruction::FMul ||
                 Opcode == Instruction::And || Opcode == Instruction::Or ||
                 Opcode == Instruction::Xor)) ||
               (Opcode == Instruction::ICmp &&
                (Kind == RK_SMin || Kind == RK_SMax ||
                 Kind == RK_UMin || Kind == RK_UMax)));
     }
public:
     /// Construction for reduced values. They are identified by opcode only and
     /// don't have associated LHS/RHS values.
     explicit OperationData(Instruction &I) {
       Opcode = I.getOpcode();
     }
 
     /// Constructor for reduction operations with opcode and its left and
     /// right operands.
-    OperationData(unsigned Opcode, Value *LHS, Value *RHS, ReductionKind Kind)
-        : Opcode(Opcode), LHS(LHS), RHS(RHS), Kind(Kind) {
+    OperationData(unsigned Opcode, ReductionKind Kind)
+        : Opcode(Opcode), Kind(Kind) {
       assert(Kind != RK_None && "One of the reduction operations is expected.");
     }
 
     explicit operator bool() const { return Opcode; }
 
     /// Return true if this operation is any kind of minimum or maximum.
     bool isMinMax() const {
       switch (Kind) {
unsigned getFirstOperandIndex() const {
       // We allow calling this before 'Kind' is set, so handle that specially.
       if (Kind == RK_None)
         return 0;
       return isMinMax() ? 1 : 0;
     }
 
     /// Total number of operands in the reduction operation.
     unsigned getNumberOfOperands() const {
-      assert(Kind != RK_None && !!*this && LHS && RHS &&
-             "Expected reduction operation.");
+      assert(Kind != RK_None && !!*this && "Expected reduction operation.");
       return isMinMax() ? 3 : 2;
     }
 
     /// Checks if the instruction is in basic block \p BB.
     /// For a min/max reduction check that both compare and select are in \p BB.
     bool hasSameParent(Instruction *I, BasicBlock *BB, bool IsRedOp) const {
-      assert(Kind != RK_None && !!*this && LHS && RHS &&
-             "Expected reduction operation.");
+      assert(Kind != RK_None && !!*this && "Expected reduction operation.");
       if (IsRedOp && isMinMax()) {
         auto *Cmp = cast<Instruction>(cast<SelectInst>(I)->getCondition());
         return I->getParent() == BB && Cmp && Cmp->getParent() == BB;
       }
       return I->getParent() == BB;
     }
 
     /// Expected number of uses for reduction operations/reduced values.
     bool hasRequiredNumberOfUses(Instruction *I, bool IsReductionOp) const {
-      assert(Kind != RK_None && !!*this && LHS && RHS &&
-             "Expected reduction operation.");
+      assert(Kind != RK_None && !!*this && "Expected reduction operation.");
       // SelectInst must be used twice while the condition op must have single
       // use only.
       if (isMinMax())
         return I->hasNUses(2) &&
                (!IsReductionOp ||
                 cast<SelectInst>(I)->getCondition()->hasOneUse());
 
       // Arithmetic reduction operation must be used once only.
       return I->hasOneUse();
     }
 
     /// Initializes the list of reduction operations.
     void initReductionOps(ReductionOpsListType &ReductionOps) {
-      assert(Kind != RK_None && !!*this && LHS && RHS &&
-             "Expected reduction operation.");
+      assert(Kind != RK_None && !!*this && "Expected reduction operation.");
       if (isMinMax())
         ReductionOps.assign(2, ReductionOpsType());
       else
         ReductionOps.assign(1, ReductionOpsType());
     }
 
     /// Add all reduction operations for the reduction instruction \p I.
     void addReductionOps(Instruction *I, ReductionOpsListType &ReductionOps) {
-      assert(Kind != RK_None && !!*this && LHS && RHS &&
-             "Expected reduction operation.");
+      assert(Kind != RK_None && !!*this && "Expected reduction operation.");
       if (isMinMax()) {
         ReductionOps[0].emplace_back(cast<SelectInst>(I)->getCondition());
         ReductionOps[1].emplace_back(I);
       } else {
         ReductionOps[0].emplace_back(I);
       }
     }
 
     /// Checks if instruction is associative and can be vectorized.
     bool isAssociative(Instruction *I) const {
-      assert(Kind != RK_None && *this && LHS && RHS &&
-             "Expected reduction operation.");
+      assert(Kind != RK_None && *this && "Expected reduction operation.");
       switch (Kind) {
       case RK_Arithmetic:
         return I->isAssociative();
       case RK_SMin:
       case RK_SMax:
       case RK_UMin:
       case RK_UMax:
         assert(Opcode == Instruction::ICmp &&
                "Only integer compare operation is expected.");
         return true;
       case RK_None:
         break;
       }
       llvm_unreachable("Reduction kind is not set");
     }
 
     /// Checks if the reduction operation can be vectorized.
     bool isVectorizable(Instruction *I) const {
       return isVectorizable() && isAssociative(I);
     }
 
     /// Checks if two operation data are both a reduction op or both a reduced
     /// value.
     bool operator==(const OperationData &OD) const {
-      assert(((Kind != OD.Kind) || ((!LHS == !OD.LHS) && (!RHS == !OD.RHS))) &&
+      assert(((Kind != OD.Kind) || (Opcode != 0 && OD.Opcode != 0)) &&
              "One of the comparing operations is incorrect.");
-      return this == &OD || (Kind == OD.Kind && Opcode == OD.Opcode);
+      return Kind == OD.Kind && Opcode == OD.Opcode;
     }
     bool operator!=(const OperationData &OD) const { return !(*this == OD); }
     void clear() {
       Opcode = 0;
-      LHS = nullptr;
-      RHS = nullptr;
       Kind = RK_None;
     }
 
     /// Get the opcode of the reduction operation.
     unsigned getOpcode() const {
       assert(isVectorizable() && "Expected vectorizable operation.");
       return Opcode;
     }
 
     /// Get kind of reduction data.
     ReductionKind getKind() const { return Kind; }
-    Value *getLHS() const { return LHS; }
-    Value *getRHS() const { return RHS; }
-    Type *getConditionType() const {
-      return isMinMax() ? CmpInst::makeCmpResultType(LHS->getType()) : nullptr;
+    Value *getLHS(Instruction *I) const {
+      if (Kind == RK_None)
+        return nullptr;
+      return I->getOperand(getFirstOperandIndex());
+    }
+    Value *getRHS(Instruction *I) const {
+      if (Kind == RK_None)
+        return nullptr;
+      return I->getOperand(getFirstOperandIndex() + 1);
     }
 
     /// Creates reduction operation with the current opcode with the IR flags
     /// from \p ReductionOps.
     Value *createOp(IRBuilder<> &Builder, Value *LHS, Value *RHS,
                     const Twine &Name,
                     const ReductionOpsListType &ReductionOps) const {
       assert(isVectorizable() &&
class HorizontalReduction {
 
   static OperationData getOperationData(Instruction *I) {
     if (!I)
       return OperationData();
 
     Value *LHS;
     Value *RHS;
     if (m_BinOp(m_Value(LHS), m_Value(RHS)).match(I)) {
-      return OperationData(cast<BinaryOperator>(I)->getOpcode(), LHS, RHS,
-                           RK_Arithmetic);
+      return OperationData(cast<BinaryOperator>(I)->getOpcode(), RK_Arithmetic);
     }
     if (auto *Select = dyn_cast<SelectInst>(I)) {
       // Look for a min/max pattern.
       if (m_UMin(m_Value(LHS), m_Value(RHS)).match(Select)) {
-        return OperationData(Instruction::ICmp, LHS, RHS, RK_UMin);
+        return OperationData(Instruction::ICmp, RK_UMin);
       } else if (m_SMin(m_Value(LHS), m_Value(RHS)).match(Select)) {
-        return OperationData(Instruction::ICmp, LHS, RHS, RK_SMin);
+        return OperationData(Instruction::ICmp, RK_SMin);
       } else if (m_UMax(m_Value(LHS), m_Value(RHS)).match(Select)) {
-        return OperationData(Instruction::ICmp, LHS, RHS, RK_UMax);
+        return OperationData(Instruction::ICmp, RK_UMax);
       } else if (m_SMax(m_Value(LHS), m_Value(RHS)).match(Select)) {
-        return OperationData(Instruction::ICmp, LHS, RHS, RK_SMax);
+        return OperationData(Instruction::ICmp, RK_SMax);
       } else {
         // Try harder: look for min/max pattern based on instructions producing
         // same values such as: select ((cmp Inst1, Inst2), Inst1, Inst2).
         // During the intermediate stages of SLP, it's very common to have
         // pattern like this (since optimizeGatherSequence is run only once
         // at the end):
         // %1 = extractelement <2 x i32> %a, i32 0
         // %2 = extractelement <2 x i32> %a, i32 1
if (auto *Select = dyn_cast<SelectInst>(I)) {
             return OperationData(*I);
         }
         switch (Pred) {
         default:
           return OperationData(*I);
 
         case CmpInst::ICMP_ULT:
         case CmpInst::ICMP_ULE:
-          return OperationData(Instruction::ICmp, LHS, RHS, RK_UMin);
+          return OperationData(Instruction::ICmp, RK_UMin);
 
         case CmpInst::ICMP_SLT:
         case CmpInst::ICMP_SLE:
-          return OperationData(Instruction::ICmp, LHS, RHS, RK_SMin);
+          return OperationData(Instruction::ICmp, RK_SMin);
 
         case CmpInst::ICMP_UGT:
         case CmpInst::ICMP_UGE:
-          return OperationData(Instruction::ICmp, LHS, RHS, RK_UMax);
+          return OperationData(Instruction::ICmp, RK_UMax);
 
         case CmpInst::ICMP_SGT:
         case CmpInst::ICMP_SGE:
-          return OperationData(Instruction::ICmp, LHS, RHS, RK_SMax);
+          return OperationData(Instruction::ICmp, RK_SMax);
         }
       }
     }
     return OperationData(*I);
   }
 
 public:
   HorizontalReduction() = default;
 
   /// Try to find a reduction tree.
   bool matchAssociativeReduction(PHINode *Phi, Instruction *B) {
     assert((!Phi || is_contained(Phi->operands(), B)) &&
            "Thi phi needs to use the binary operator");
 
     ReductionData = getOperationData(B);
 
     // We could have a initial reductions that is not an add.
     //  r *= v1 + v2 + v3 + v4
     // In such a case start looking for a tree rooted in the first '+'.
     if (Phi) {
-      if (ReductionData.getLHS() == Phi) {
+      if (ReductionData.getLHS(B) == Phi) {
         Phi = nullptr;
-        B = dyn_cast<Instruction>(ReductionData.getRHS());
+        B = dyn_cast<Instruction>(ReductionData.getRHS(B));
         ReductionData = getOperationData(B);
-      } else if (ReductionData.getRHS() == Phi) {
+      } else if (ReductionData.getRHS(B) == Phi) {
         Phi = nullptr;
-        B = dyn_cast<Instruction>(ReductionData.getLHS());
+        B = dyn_cast<Instruction>(ReductionData.getLHS(B));
         ReductionData = getOperationData(B);
       }
     }
 
     if (!ReductionData.isVectorizable(B))
       return false;
 
     Type *Ty = B->getType();