[ConstantHoisting] use struct rather than tuple for adjustments

[nickdesaulniers]

Summary

We pack this info in a tuple just to spread it back out for a function
call. Spreads in C++ are awkward. If I want to add an additional
element to the tuple, I need to add more calls to std::get<> later. Just
use a struct.

[void]

I'm not familiar with this code, but why not place the information into ConstantUser instead? Or maybe make the struct a private member of ConstantHoistingPass.

[nickdesaulniers]

• make UserAdjustment private to ConstantHoistingPass as per @void

[nickdesaulniers]

In D155236#4501902, @void wrote:

I'm not familiar with this code,

Are you still comfortable reviewing simple NFC refactorings?

but why not place the information into ConstantUser instead?

I'm sure it's possible, but this commit is simplying replacing the RebasedUse std::tuple with a dedicated class. We may be able to refactor that further, but that's more than I care to do for this code, as D155237 is more important to me and I wanted to factor these changes out of D155237. Would you prefer such a more involved change though?

Or maybe make the struct a private member of ConstantHoistingPass.

Sure, good idea! Done. PTAL

[void]

Bueno!

[nickdesaulniers]

whoops, I messed up the rebase; MatInsertPt member should go in the child commit; let me fix that up.

[nickdesaulniers]

• fix rebase

llvm/include/llvm/Transforms/Scalar/ConstantHoisting.h

void findAndMakeBaseConstant(ConstCandVecType::iterator S,
                                ConstCandVecType::iterator E,
       SmallVectorImpl<consthoist::ConstantInfo> &ConstInfoVec);
   unsigned maximizeConstantsInRange(ConstCandVecType::iterator S,
                                     ConstCandVecType::iterator E,
                                     ConstCandVecType::iterator &MaxCostItr);
   // If BaseGV is nullptr, find base among Constant Integer candidates;
   // otherwise find base among constant GEPs sharing BaseGV as base pointer.
   void findBaseConstants(GlobalVariable *BaseGV);
-  void emitBaseConstants(Instruction *Base, Constant *Offset, Type *Ty,
-                         const consthoist::ConstantUser &ConstUser);
+
+  /// A ConstantUser grouped with the Type and Constant adjustment. The user
+  /// will be adjusted by Offset.
+  struct UserAdjustment {
+    Constant *Offset;
+    Type *Ty;
+    const consthoist::ConstantUser User;
+    UserAdjustment(Constant *O, Type *T, consthoist::ConstantUser U)
+        : Offset(O), Ty(T), User(U) {}
+  };
+  void emitBaseConstants(Instruction *Base, UserAdjustment *Adj);
   // If BaseGV is nullptr, emit Constant Integer base; otherwise emit
   // constant GEP base.
   bool emitBaseConstants(GlobalVariable *BaseGV);
   void deleteDeadCastInst() const;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_SCALAR_CONSTANTHOISTING_H

llvm/lib/Transforms/Scalar/ConstantHoisting.cpp

static bool updateOperand(Instruction *Inst, unsigned Idx, Instruction *Mat) {
 
   Inst->setOperand(Idx, Mat);
   return true;
 }
 
 /// Emit materialization code for all rebased constants and update their
 /// users.
 void ConstantHoistingPass::emitBaseConstants(Instruction *Base,
-                                             Constant *Offset,
-                                             Type *Ty,
-                                             const ConstantUser &ConstUser) {
+                                             UserAdjustment *Adj) {
   Instruction *Mat = Base;
 
   // The same offset can be dereferenced to different types in nested struct.
-  if (!Offset && Ty && Ty != Base->getType())
-    Offset = ConstantInt::get(Type::getInt32Ty(*Ctx), 0);
+  if (!Adj->Offset && Adj->Ty && Adj->Ty != Base->getType())
+    Adj->Offset = ConstantInt::get(Type::getInt32Ty(*Ctx), 0);
 
-  if (Offset) {
-    Instruction *InsertionPt = findMatInsertPt(ConstUser.Inst,
-                                               ConstUser.OpndIdx);
-    if (Ty) {
+  if (Adj->Offset) {
+    Instruction *InsertionPt =
+        findMatInsertPt(Adj->User.Inst, Adj->User.OpndIdx);
+    if (Adj->Ty) {
       // Constant being rebased is a ConstantExpr.
-      PointerType *Int8PtrTy = Type::getInt8PtrTy(*Ctx,
-          cast<PointerType>(Ty)->getAddressSpace());
+      PointerType *Int8PtrTy = Type::getInt8PtrTy(
+          *Ctx, cast<PointerType>(Adj->Ty)->getAddressSpace());
       Base = new BitCastInst(Base, Int8PtrTy, "base_bitcast", InsertionPt);
-      Mat = GetElementPtrInst::Create(Type::getInt8Ty(*Ctx), Base,
-          Offset, "mat_gep", InsertionPt);
-      Mat = new BitCastInst(Mat, Ty, "mat_bitcast", InsertionPt);
+      Mat = GetElementPtrInst::Create(Type::getInt8Ty(*Ctx), Base, Adj->Offset,
+                                      "mat_gep", InsertionPt);
+      Mat = new BitCastInst(Mat, Adj->Ty, "mat_bitcast", InsertionPt);
     } else
       // Constant being rebased is a ConstantInt.
-      Mat = BinaryOperator::Create(Instruction::Add, Base, Offset,
+      Mat = BinaryOperator::Create(Instruction::Add, Base, Adj->Offset,
                                    "const_mat", InsertionPt);
 
     LLVM_DEBUG(dbgs() << "Materialize constant (" << *Base->getOperand(0)
-                      << " + " << *Offset << ") in BB "
+                      << " + " << *Adj->Offset << ") in BB "
                       << Mat->getParent()->getName() << '\n'
                       << *Mat << '\n');
-    Mat->setDebugLoc(ConstUser.Inst->getDebugLoc());
+    Mat->setDebugLoc(Adj->User.Inst->getDebugLoc());
   }
-  Value *Opnd = ConstUser.Inst->getOperand(ConstUser.OpndIdx);
+  Value *Opnd = Adj->User.Inst->getOperand(Adj->User.OpndIdx);
 
   // Visit constant integer.
   if (isa<ConstantInt>(Opnd)) {
-    LLVM_DEBUG(dbgs() << "Update: " << *ConstUser.Inst << '\n');
-    if (!updateOperand(ConstUser.Inst, ConstUser.OpndIdx, Mat) && Offset)
+    LLVM_DEBUG(dbgs() << "Update: " << *Adj->User.Inst << '\n');
+    if (!updateOperand(Adj->User.Inst, Adj->User.OpndIdx, Mat) && Adj->Offset)
       Mat->eraseFromParent();
-    LLVM_DEBUG(dbgs() << "To    : " << *ConstUser.Inst << '\n');
+    LLVM_DEBUG(dbgs() << "To    : " << *Adj->User.Inst << '\n');
     return;
   }
 
   // Visit cast instruction.
   if (auto CastInst = dyn_cast<Instruction>(Opnd)) {
     assert(CastInst->isCast() && "Expected an cast instruction!");
     // Check if we already have visited this cast instruction before to avoid
     // unnecessary cloning.
     Instruction *&ClonedCastInst = ClonedCastMap[CastInst];
     if (!ClonedCastInst) {
       ClonedCastInst = CastInst->clone();
       ClonedCastInst->setOperand(0, Mat);
       ClonedCastInst->insertAfter(CastInst);
       // Use the same debug location as the original cast instruction.
       ClonedCastInst->setDebugLoc(CastInst->getDebugLoc());
       LLVM_DEBUG(dbgs() << "Clone instruction: " << *CastInst << '\n'
                         << "To               : " << *ClonedCastInst << '\n');
     }
 
-    LLVM_DEBUG(dbgs() << "Update: " << *ConstUser.Inst << '\n');
-    updateOperand(ConstUser.Inst, ConstUser.OpndIdx, ClonedCastInst);
-    LLVM_DEBUG(dbgs() << "To    : " << *ConstUser.Inst << '\n');
+    LLVM_DEBUG(dbgs() << "Update: " << *Adj->User.Inst << '\n');
+    updateOperand(Adj->User.Inst, Adj->User.OpndIdx, ClonedCastInst);
+    LLVM_DEBUG(dbgs() << "To    : " << *Adj->User.Inst << '\n');
     return;
   }
 
   // Visit constant expression.
   if (auto ConstExpr = dyn_cast<ConstantExpr>(Opnd)) {
     if (isa<GEPOperator>(ConstExpr)) {
       // Operand is a ConstantGEP, replace it.
-      updateOperand(ConstUser.Inst, ConstUser.OpndIdx, Mat);
+      updateOperand(Adj->User.Inst, Adj->User.OpndIdx, Mat);
       return;
     }
 
     // Aside from constant GEPs, only constant cast expressions are collected.
     assert(ConstExpr->isCast() && "ConstExpr should be a cast");
     Instruction *ConstExprInst = ConstExpr->getAsInstruction(
-        findMatInsertPt(ConstUser.Inst, ConstUser.OpndIdx));
+        findMatInsertPt(Adj->User.Inst, Adj->User.OpndIdx));
     ConstExprInst->setOperand(0, Mat);
 
     // Use the same debug location as the instruction we are about to update.
-    ConstExprInst->setDebugLoc(ConstUser.Inst->getDebugLoc());
+    ConstExprInst->setDebugLoc(Adj->User.Inst->getDebugLoc());
 
     LLVM_DEBUG(dbgs() << "Create instruction: " << *ConstExprInst << '\n'
                       << "From              : " << *ConstExpr << '\n');
-    LLVM_DEBUG(dbgs() << "Update: " << *ConstUser.Inst << '\n');
-    if (!updateOperand(ConstUser.Inst, ConstUser.OpndIdx, ConstExprInst)) {
+    LLVM_DEBUG(dbgs() << "Update: " << *Adj->User.Inst << '\n');
+    if (!updateOperand(Adj->User.Inst, Adj->User.OpndIdx, ConstExprInst)) {
       ConstExprInst->eraseFromParent();
-      if (Offset)
+      if (Adj->Offset)
         Mat->eraseFromParent();
     }
-    LLVM_DEBUG(dbgs() << "To    : " << *ConstUser.Inst << '\n');
+    LLVM_DEBUG(dbgs() << "To    : " << *Adj->User.Inst << '\n');
     return;
   }
 }
 
 /// Hoist and hide the base constant behind a bitcast and emit
 /// materialization code for derived constants.
 bool ConstantHoistingPass::emitBaseConstants(GlobalVariable *BaseGV) {
   bool MadeChange = false;
   SmallVectorImpl<consthoist::ConstantInfo> &ConstInfoVec =
       BaseGV ? ConstGEPInfoMap[BaseGV] : ConstIntInfoVec;
   for (auto const &ConstInfo : ConstInfoVec) {
     SetVector<Instruction *> IPSet = findConstantInsertionPoint(ConstInfo);
     // We can have an empty set if the function contains unreachable blocks.
     if (IPSet.empty())
       continue;
 
     unsigned UsesNum = 0;
     unsigned ReBasesNum = 0;
     unsigned NotRebasedNum = 0;
     for (Instruction *IP : IPSet) {
       // First, collect constants depending on this IP of the base.
       UsesNum = 0;
-      using RebasedUse = std::tuple<Constant *, Type *, ConstantUser>;
-      SmallVector<RebasedUse, 4> ToBeRebased;
+      SmallVector<UserAdjustment, 4> ToBeRebased;
       for (auto const &RCI : ConstInfo.RebasedConstants) {
         UsesNum += RCI.Uses.size();
         for (auto const &U : RCI.Uses) {
           BasicBlock *OrigMatInsertBB =
               findMatInsertPt(U.Inst, U.OpndIdx)->getParent();
           // If Base constant is to be inserted in multiple places,
           // generate rebase for U using the Base dominating U.
           if (IPSet.size() == 1 ||
               DT->dominates(IP->getParent(), OrigMatInsertBB))
-            ToBeRebased.push_back(RebasedUse(RCI.Offset, RCI.Ty, U));
+            ToBeRebased.emplace_back(RCI.Offset, RCI.Ty, U);
         }
       }
 
       // If only few constants depend on this IP of base, skip rebasing,
       // assuming the base and the rebased have the same materialization cost.
       if (ToBeRebased.size() < MinNumOfDependentToRebase) {
         NotRebasedNum += ToBeRebased.size();
         continue;
for (Instruction *IP : IPSet) {
 
       Base->setDebugLoc(IP->getDebugLoc());
 
       LLVM_DEBUG(dbgs() << "Hoist constant (" << *ConstInfo.BaseInt
                         << ") to BB " << IP->getParent()->getName() << '\n'
                         << *Base << '\n');
 
       // Emit materialization code for rebased constants depending on this IP.
-      for (auto const &R : ToBeRebased) {
-        Constant *Off = std::get<0>(R);
-        Type *Ty = std::get<1>(R);
-        ConstantUser U = std::get<2>(R);
-        emitBaseConstants(Base, Off, Ty, U);
+      for (UserAdjustment &R : ToBeRebased) {
+        emitBaseConstants(Base, &R);
         ReBasesNum++;
         // Use the same debug location as the last user of the constant.
         Base->setDebugLoc(DILocation::getMergedLocation(
-            Base->getDebugLoc(), U.Inst->getDebugLoc()));
+            Base->getDebugLoc(), R.User.Inst->getDebugLoc()));
       }
       assert(!Base->use_empty() && "The use list is empty!?");
       assert(isa<Instruction>(Base->user_back()) &&
              "All uses should be instructions.");
     }
     (void)UsesNum;
     (void)ReBasesNum;
     (void)NotRebasedNum;