[NFC][AMDGPULowerModuleLDSPass] Factorize repetead sort code

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp

249

The danger of anonymous values may require a stable_sort (or do we error on those here, I forget)

[JonChesterfield]

The move -> return approach seems reasonable. Could we go with GlobalVariable* instead of T before landing?

llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp

249

No anonymous variables here - these globals are all created by this pass, and all given non-empty names.

There's a hard error on anonymous functions at present, can drop that if we find a better way to bind structs to functions

1318

This is a bit dubious in general, should probably be establishing the sorted invariant before calling into this. For another patch.

[JonChesterfield]

llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp

249

Re anonymous vars - we should test that. This may be reachable from the call that chooses an order for the array<i32> subtable with the original variable names.

The variables in question are deleted by this pass so could be assigned arbitrary unique names as a fix if necessary.

llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp

bool isKernelLDS(const Function *F) {
   // isModuleEntryFunction which returns true for more calling conventions
   // than AMDGPU::isKernel does. There's a FIXME on AMDGPU::isKernel.
   // There's also a test that checks that the LDS lowering does not hit on
   // a graphics shader, denoted amdgpu_ps, so stay with the limited case.
   // Putting LDS in the name of the function to draw attention to this.
   return AMDGPU::isKernel(F->getCallingConv());
 }
 
+template <typename T> std::vector<T> sortByName(std::vector<T> &&V) {
+  llvm::sort(V.begin(), V.end(), [](const auto *L, const auto *R) {
+    return L->getName() < R->getName();
+  });
+  return V;
+}
+
 class AMDGPULowerModuleLDS : public ModulePass {
 
   static void
   removeLocalVarsFromUsedLists(Module &M,
                                const DenseSet<GlobalVariable *> &LocalVars) {
     // The verifier rejects used lists containing an inttoptr of a constant
     // so remove the variables from these lists before replaceAllUsesWith
     SmallPtrSet<Constant *, 8> LocalVarsSet;
if (!KernelsThatAllocateTableLDS.empty() ||
         if (KernelsThatAllocateTableLDS.contains(&Func) ||
             KernelsThatIndirectlyAllocateDynamicLDS.contains(&Func)) {
           assert(Func.hasName()); // else fatal error earlier
           OrderedKernels.push_back(&Func);
         }
       }
 
       // Put them in an arbitrary but reproducible order
-      llvm::sort(OrderedKernels.begin(), OrderedKernels.end(),
-                 [](const Function *lhs, const Function *rhs) -> bool {
-                   return lhs->getName() < rhs->getName();
-                 });
+      OrderedKernels = sortByName(std::move(OrderedKernels));
 
       // Annotate the kernels with their order in this vector
       LLVMContext &Ctx = M->getContext();
       IRBuilder<> Builder(Ctx);
 
       if (OrderedKernels.size() > UINT32_MAX) {
         // 32 bit keeps it in one SGPR. > 2**32 kernels won't fit on the GPU
         report_fatal_error("Unimplemented LDS lowering for > 2**32 kernels");
if (!KernelsThatAllocateTableLDS.empty()) {
 
       for (size_t i = 0; i < OrderedKernels.size(); i++) {
         markUsedByKernel(Builder, OrderedKernels[i],
                          KernelToReplacement[OrderedKernels[i]].SGV);
       }
 
       // The order must be consistent between lookup table and accesses to
       // lookup table
-      std::vector<GlobalVariable *> TableLookupVariablesOrdered(
-          TableLookupVariables.begin(), TableLookupVariables.end());
-      llvm::sort(TableLookupVariablesOrdered.begin(),
-                 TableLookupVariablesOrdered.end(),
-                 [](const GlobalVariable *lhs, const GlobalVariable *rhs) {
-                   return lhs->getName() < rhs->getName();
-                 });
+      auto TableLookupVariablesOrdered = sortByName(std::vector(
+          TableLookupVariables.begin(), TableLookupVariables.end()));
 
       GlobalVariable *LookupTable = buildLookupTable(
           M, TableLookupVariablesOrdered, OrderedKernels, KernelToReplacement);
       replaceUsesInInstructionsWithTableLookup(M, TableLookupVariablesOrdered,
                                                LookupTable);
     }
 
     DenseMap<Function *, GlobalVariable *> KernelToCreatedDynamicLDS =
for (auto &GV : M.globals()) {
         GV.setAlignment(Alignment);
       }
     }
     return Changed;
   }
 
   static LDSVariableReplacement createLDSVariableReplacement(
       Module &M, std::string VarName,
       DenseSet<GlobalVariable *> const &LDSVarsToTransform) {
     // Create a struct instance containing LDSVarsToTransform and map from those
     // variables to ConstantExprGEP
     // Variables may be introduced to meet alignment requirements. No aliasing
     // metadata is useful for these as they have no uses. Erased before return.
 
     LLVMContext &Ctx = M.getContext();
     const DataLayout &DL = M.getDataLayout();
     assert(!LDSVarsToTransform.empty());
 
     SmallVector<OptimizedStructLayoutField, 8> LayoutFields;
     LayoutFields.reserve(LDSVarsToTransform.size());
     {
       // The order of fields in this struct depends on the order of
       // varables in the argument which varies when changing how they
       // are identified, leading to spurious test breakage.
-      std::vector<GlobalVariable *> Sorted(LDSVarsToTransform.begin(),
-                                           LDSVarsToTransform.end());
-      llvm::sort(Sorted.begin(), Sorted.end(),
-                 [](const GlobalVariable *lhs, const GlobalVariable *rhs) {
-                   return lhs->getName() < rhs->getName();
-                 });
+      auto Sorted = sortByName(
+          std::vector(LDSVarsToTransform.begin(), LDSVarsToTransform.end()));
+
       for (GlobalVariable *GV : Sorted) {
         OptimizedStructLayoutField F(GV,
                                      DL.getTypeAllocSize(GV->getValueType()),
                                      AMDGPU::getAlign(DL, GV));
         LayoutFields.emplace_back(F);
       }
     }
 
static LDSVariableReplacement createLDSVariableReplacement(
     }
     assert(Map.size() == LDSVarsToTransform.size());
     return {SGV, std::move(Map)};
   }
 
   template <typename PredicateTy>
   static void replaceLDSVariablesWithStruct(
       Module &M, DenseSet<GlobalVariable *> const &LDSVarsToTransformArg,
-      LDSVariableReplacement Replacement, PredicateTy Predicate) {
+      const LDSVariableReplacement &Replacement, PredicateTy Predicate) {
     LLVMContext &Ctx = M.getContext();
     const DataLayout &DL = M.getDataLayout();
 
     // A hack... we need to insert the aliasing info in a predictable order for
     // lit tests. Would like to have them in a stable order already, ideally the
     // same order they get allocated, which might mean an ordered set container
-    std::vector<GlobalVariable *> LDSVarsToTransform(
-        LDSVarsToTransformArg.begin(), LDSVarsToTransformArg.end());
-    llvm::sort(LDSVarsToTransform.begin(), LDSVarsToTransform.end(),
-               [](const GlobalVariable *lhs, const GlobalVariable *rhs) {
-                 return lhs->getName() < rhs->getName();
-               });
+    std::vector<GlobalVariable *> LDSVarsToTransform = sortByName(std::vector(
+        LDSVarsToTransformArg.begin(), LDSVarsToTransformArg.end()));
 
     // Create alias.scope and their lists. Each field in the new structure
     // does not alias with all other fields.
     SmallVector<MDNode *> AliasScopes;
     SmallVector<Metadata *> NoAliasList;
     const size_t NumberVars = LDSVarsToTransform.size();
     if (NumberVars > 1) {
       MDBuilder MDB(Ctx);