[ArgPromotion] Rename variables according to the code style. NFC

[psamolysov]

Summary

Some loop counters ('i', 'e') and variables ('type') were named not in
accordance with the code style and clang-tidy issues warnings about
the using of such variables. This patch renames the variables and fixes
some typos in the comments within the source file.

[psamolysov]

Another typo in a comment has been fixed as well.

[jdoerfert]

Thx! LG, one nit, see below.

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

365

Make it I or something, not N. N is (for me) associated with the upper bound, also above.

[psamolysov]

@jdoerfert Thank you for the comment. I would like to use a single-letter name in order not to reformat the code. What about J? If J doesn't sound good, I can propose M from member because this variable is an index of an element from a struct.

[psamolysov]

Rename N to J.

[psamolysov]

Rebased on the current main branch.

[psamolysov]

Colleagues, if you have no objections against the patch, could you help me with landing? I have no commit rights and cannot land the patch.

[nikic]

In D123662#3456391, @psamolysov wrote:

Colleagues, if you have no objections against the patch, could you help me with landing? I have no commit rights and cannot land the patch.

Can you please share the Name <email> to use for the commit?

[psamolysov]

@nikic Thank you very much. Please, use Pavel Samolysov <[e-mail]>

llvm/include/llvm/Transforms/IPO/ArgumentPromotion.h

 /// direct (by-value) arguments.
 class ArgumentPromotionPass : public PassInfoMixin<ArgumentPromotionPass> {
   unsigned MaxElements;
 
 public:
   ArgumentPromotionPass(unsigned MaxElements = 3u) : MaxElements(MaxElements) {}
 
   /// Checks if a type could have padding bytes.
-  static bool isDenselyPacked(Type *type, const DataLayout &DL);
+  static bool isDenselyPacked(Type *Ty, const DataLayout &DL);
 
   PreservedAnalyses run(LazyCallGraph::SCC &C, CGSCCAnalysisManager &AM,
                         LazyCallGraph &CG, CGSCCUpdateResult &UR);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_IPO_ARGUMENTPROMOTION_H

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

struct ArgPart {
   /// A representative guaranteed-executed load instruction for use by
   /// metadata transfer.
   LoadInst *MustExecLoad;
 };
 using OffsetAndArgPart = std::pair<int64_t, ArgPart>;
 
 static Value *createByteGEP(IRBuilderBase &IRB, const DataLayout &DL,
                             Value *Ptr, Type *ResElemTy, int64_t Offset) {
-  // For non-opaque pointers, try create a "nice" GEP if possible, otherwise
+  // For non-opaque pointers, try to create a "nice" GEP if possible, otherwise
   // fall back to an i8 GEP to a specific offset.
   unsigned AddrSpace = Ptr->getType()->getPointerAddressSpace();
   APInt OrigOffset(DL.getIndexTypeSizeInBits(Ptr->getType()), Offset);
   if (!Ptr->getType()->isOpaquePointerTy()) {
     Type *OrigElemTy = Ptr->getType()->getNonOpaquePointerElementType();
     if (OrigOffset == 0 && OrigElemTy == ResElemTy)
       return Ptr;
 
static Function *doPromotion(
   // Create the new function body and insert it into the module.
   Function *NF = Function::Create(NFTy, F->getLinkage(), F->getAddressSpace(),
                                   F->getName());
   NF->copyAttributesFrom(F);
   NF->copyMetadata(F, 0);
 
   // The new function will have the !dbg metadata copied from the original
   // function. The original function may not be deleted, and dbg metadata need
-  // to be unique so we need to drop it.
+  // to be unique, so we need to drop it.
   F->setSubprogram(nullptr);
 
   LLVM_DEBUG(dbgs() << "ARG PROMOTION:  Promoting to:" << *NF << "\n"
                     << "From: " << *F);
 
   // Recompute the parameter attributes list based on the new arguments for
   // the function.
   NF->setAttributes(AttributeList::get(F->getContext(), PAL.getFnAttrs(),
static Function *doPromotion(
   while (!F->use_empty()) {
     CallBase &CB = cast<CallBase>(*F->user_back());
     assert(CB.getCalledFunction() == F);
     const AttributeList &CallPAL = CB.getAttributes();
     IRBuilder<NoFolder> IRB(&CB);
 
     // Loop over the operands, inserting GEP and loads in the caller as
     // appropriate.
-    auto AI = CB.arg_begin();
+    auto *AI = CB.arg_begin();
     ArgNo = 0;
     for (Function::arg_iterator I = F->arg_begin(), E = F->arg_end(); I != E;
          ++I, ++AI, ++ArgNo)
       if (!ArgsToPromote.count(&*I) && !ByValArgsToTransform.count(&*I)) {
         Args.push_back(*AI); // Unmodified argument
         ArgAttrVec.push_back(CallPAL.getParamAttrs(ArgNo));
       } else if (ByValArgsToTransform.count(&*I)) {
         // Emit a GEP and load for each element of the struct.
         Type *AgTy = I->getParamByValType();
         StructType *STy = cast<StructType>(AgTy);
         Value *Idxs[2] = {
             ConstantInt::get(Type::getInt32Ty(F->getContext()), 0), nullptr};
         const StructLayout *SL = DL.getStructLayout(STy);
         Align StructAlign = *I->getParamAlign();
-        for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {
-          Idxs[1] = ConstantInt::get(Type::getInt32Ty(F->getContext()), i);
+        for (unsigned N = 0, Elems = STy->getNumElements(); N != Elems; ++N) {
+          Idxs[1] = ConstantInt::get(Type::getInt32Ty(F->getContext()), N);
           auto *Idx =
-              IRB.CreateGEP(STy, *AI, Idxs, (*AI)->getName() + "." + Twine(i));
+              IRB.CreateGEP(STy, *AI, Idxs, (*AI)->getName() + "." + Twine(N));
           // TODO: Tell AA about the new values?
           Align Alignment =
-              commonAlignment(StructAlign, SL->getElementOffset(i));
+              commonAlignment(StructAlign, SL->getElementOffset(N));
           Args.push_back(IRB.CreateAlignedLoad(
-              STy->getElementType(i), Idx, Alignment, Idx->getName() + ".val"));
+              STy->getElementType(N), Idx, Alignment, Idx->getName() + ".val"));
           ArgAttrVec.push_back(AttributeSet());
         }
       } else if (!I->use_empty()) {
         Value *V = *AI;
         const auto &ArgParts = ArgsToPromote.find(&*I)->second;
         for (const auto &Pair : ArgParts) {
           LoadInst *LI = IRB.CreateAlignedLoad(
               Pair.second.Ty,
if (ByValArgsToTransform.count(&Arg)) {
       Align StructAlign = *Arg.getParamAlign();
       Value *TheAlloca = new AllocaInst(AgTy, DL.getAllocaAddrSpace(), nullptr,
                                         StructAlign, "", InsertPt);
       StructType *STy = cast<StructType>(AgTy);
       Value *Idxs[2] = {ConstantInt::get(Type::getInt32Ty(F->getContext()), 0),
                         nullptr};
       const StructLayout *SL = DL.getStructLayout(STy);
 
-      for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {
-        Idxs[1] = ConstantInt::get(Type::getInt32Ty(F->getContext()), i);
+      for (unsigned N = 0, Elems = STy->getNumElements(); N != Elems; ++N) {
+        Idxs[1] = ConstantInt::get(Type::getInt32Ty(F->getContext()), N);
         Value *Idx = GetElementPtrInst::Create(
-            AgTy, TheAlloca, Idxs, TheAlloca->getName() + "." + Twine(i),
+            AgTy, TheAlloca, Idxs, TheAlloca->getName() + "." + Twine(N),
             InsertPt);
-        I2->setName(Arg.getName() + "." + Twine(i));
-        Align Alignment = commonAlignment(StructAlign, SL->getElementOffset(i));
+        I2->setName(Arg.getName() + "." + Twine(N));
+        Align Alignment = commonAlignment(StructAlign, SL->getElementOffset(N));
         new StoreInst(&*I2++, Idx, false, Alignment, InsertPt);
       }
 
       // Anything that used the arg should now use the alloca.
       Arg.replaceAllUsesWith(TheAlloca);
       TheAlloca->takeName(&Arg);
       continue;
     }
 
auto HandleLoad = [&](LoadInst *LI,
     // For now, we only support loading one specific type at a given offset.
     if (Part.Ty != Ty) {
       LLVM_DEBUG(dbgs() << "ArgPromotion of " << *Arg << " failed: "
                         << "loaded via both " << *Part.Ty << " and " << *Ty
                         << " at offset " << Off << "\n");
       return false;
     }
 
-    // If this load is not guaranteed to execute and we haven't seen a load at
+    // If this load is not guaranteed to execute, and we haven't seen a load at
     // this offset before (or it had lower alignment), then we need to remember
     // that requirement.
     // Note that skipping loads of previously seen offsets is only correct
     // because we only allow a single type for a given offset, which also means
     // that the number of accessed bytes will be the same.
     if (!GuaranteedToExecute &&
         (OffsetNotSeenBefore || Part.Alignment < LI->getAlign())) {
       // We won't be able to prove dereferenceability for negative offsets.
static bool findArgParts(Argument *Arg, const DataLayout &DL, AAResults &AAR,
   int64_t Offset = ArgPartsVec[0].first;
   for (const auto &Pair : ArgPartsVec) {
     if (Pair.first < Offset)
       return false; // Overlap with previous part.
 
     Offset = Pair.first + DL.getTypeStoreSize(Pair.second.Ty);
   }
 
-  // Okay, now we know that the argument is only used by load instructions and
+  // Okay, now we know that the argument is only used by load instructions, and
   // it is safe to unconditionally perform all of them. Use alias analysis to
   // check to see if the pointer is guaranteed to not be modified from entry of
   // the function to each of the load instructions.
 
   // Because there could be several/many load instructions, remember which
   // blocks we know to be transparent to the load.
   df_iterator_default_set<BasicBlock *, 16> TranspBlocks;
 
static bool findArgParts(Argument *Arg, const DataLayout &DL, AAResults &AAR,
   }
 
   // If the path from the entry of the function to each load is free of
   // instructions that potentially invalidate the load, we can make the
   // transformation!
   return true;
 }
 
-bool ArgumentPromotionPass::isDenselyPacked(Type *type, const DataLayout &DL) {
+bool ArgumentPromotionPass::isDenselyPacked(Type *Ty, const DataLayout &DL) {
   // There is no size information, so be conservative.
-  if (!type->isSized())
+  if (!Ty->isSized())
     return false;
 
   // If the alloc size is not equal to the storage size, then there are padding
   // bytes. For x86_fp80 on x86-64, size: 80 alloc size: 128.
-  if (DL.getTypeSizeInBits(type) != DL.getTypeAllocSizeInBits(type))
+  if (DL.getTypeSizeInBits(Ty) != DL.getTypeAllocSizeInBits(Ty))
     return false;
 
   // FIXME: This isn't the right way to check for padding in vectors with
   // non-byte-size elements.
-  if (VectorType *seqTy = dyn_cast<VectorType>(type))
-    return isDenselyPacked(seqTy->getElementType(), DL);
+  if (VectorType *SeqTy = dyn_cast<VectorType>(Ty))
+    return isDenselyPacked(SeqTy->getElementType(), DL);
 
   // For array types, check for padding within members.
-  if (ArrayType *seqTy = dyn_cast<ArrayType>(type))
-    return isDenselyPacked(seqTy->getElementType(), DL);
+  if (ArrayType *SeqTy = dyn_cast<ArrayType>(Ty))
+    return isDenselyPacked(SeqTy->getElementType(), DL);
 
-  if (!isa<StructType>(type))
+  if (!isa<StructType>(Ty))
     return true;
 
   // Check for padding within and between elements of a struct.
-  StructType *StructTy = cast<StructType>(type);
+  StructType *StructTy = cast<StructType>(Ty);
   const StructLayout *Layout = DL.getStructLayout(StructTy);
   uint64_t StartPos = 0;
-  for (unsigned i = 0, E = StructTy->getNumElements(); i < E; ++i) {
-    Type *ElTy = StructTy->getElementType(i);
+  for (unsigned I = 0, E = StructTy->getNumElements(); I < E; ++I) {
+    Type *ElTy = StructTy->getElementType(I);
     if (!isDenselyPacked(ElTy, DL))
       return false;
-    if (StartPos != Layout->getElementOffsetInBits(i))
+    if (StartPos != Layout->getElementOffsetInBits(I))
       return false;
     StartPos += DL.getTypeAllocSizeInBits(ElTy);
   }
 
   return true;
 }
 
 /// Checks if the padding bytes of an argument could be accessed.
-static bool canPaddingBeAccessed(Argument *arg) {
-  assert(arg->hasByValAttr());
+static bool canPaddingBeAccessed(Argument *Arg) {
+  assert(Arg->hasByValAttr());
 
   // Track all the pointers to the argument to make sure they are not captured.
   SmallPtrSet<Value *, 16> PtrValues;
-  PtrValues.insert(arg);
+  PtrValues.insert(Arg);
 
   // Track all of the stores.
   SmallVector<StoreInst *, 16> Stores;
 
   // Scan through the uses recursively to make sure the pointer is always used
   // sanely.
-  SmallVector<Value *, 16> WorkList(arg->users());
+  SmallVector<Value *, 16> WorkList(Arg->users());
   while (!WorkList.empty()) {
     Value *V = WorkList.pop_back_val();
     if (isa<GetElementPtrInst>(V) || isa<PHINode>(V)) {
       if (PtrValues.insert(V).second)
         llvm::append_range(WorkList, V->users());
     } else if (StoreInst *Store = dyn_cast<StoreInst>(V)) {
       Stores.push_back(Store);
     } else if (!isa<LoadInst>(V)) {
for (Use &U : F->uses()) {
     if (CB == nullptr || !CB->isCallee(&U) ||
         CB->getFunctionType() != F->getFunctionType())
       return nullptr;
 
     // Can't change signature of musttail callee
     if (CB->isMustTailCall())
       return nullptr;
 
-    if (CB->getParent()->getParent() == F)
+    if (CB->getFunction() == F)
       IsRecursive = true;
   }
 
   // Can't change signature of musttail caller
   // FIXME: Support promoting whole chain of musttail functions
   for (BasicBlock &BB : *F)
     if (BB.getTerminatingMustTailCall())
       return nullptr;
for (Argument *PtrArg : PointerArgs) {
 
     // If this is a byval argument, and if the aggregate type is small, just
     // pass the elements, which is always safe, if the passed value is densely
     // packed or if we can prove the padding bytes are never accessed.
     //
     // Only handle arguments with specified alignment; if it's unspecified, the
     // actual alignment of the argument is target-specific.
     Type *ByValTy = PtrArg->getParamByValType();
-    bool isSafeToPromote =
+    bool IsSafeToPromote =
         ByValTy && PtrArg->getParamAlign() &&
         (ArgumentPromotionPass::isDenselyPacked(ByValTy, DL) ||
          !canPaddingBeAccessed(PtrArg));
-    if (isSafeToPromote) {
+    if (IsSafeToPromote) {
       if (StructType *STy = dyn_cast<StructType>(ByValTy)) {
         if (MaxElements > 0 && STy->getNumElements() > MaxElements) {
-          LLVM_DEBUG(dbgs() << "argpromotion disable promoting argument '"
+          LLVM_DEBUG(dbgs() << "ArgPromotion disables promoting argument '"
                             << PtrArg->getName()
                             << "' because it would require adding more"
                             << " than " << MaxElements
                             << " arguments to the function.\n");
           continue;
         }
 
         SmallVector<Type *, 4> Types;
for (CallGraphNode *OldNode : SCC) {
         // Update the call graph for the newly promoted function.
         CallGraphNode *NewNode = CG.getOrInsertFunction(NewF);
         NewNode->stealCalledFunctionsFrom(OldNode);
         if (OldNode->getNumReferences() == 0)
           delete CG.removeFunctionFromModule(OldNode);
         else
           OldF->setLinkage(Function::ExternalLinkage);
 
-        // And updat ethe SCC we're iterating as well.
+        // And update the SCC we're iterating as well.
         SCC.ReplaceNode(OldNode, NewNode);
       }
     }
     // Remember that we changed something.
     Changed |= LocalChange;
   } while (LocalChange);
 
   return Changed;
 }
 
 bool ArgPromotion::doInitialization(CallGraph &CG) {
   return CallGraphSCCPass::doInitialization(CG);
 }