Fix some of Matt's comments

Author: JonChesterfield

clang/lib/CodeGen/Targets/AMDGPU.cpp

@@ -117,8 +117,10 @@ Address AMDGPUABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                                  QualType Ty) const {
   const bool IsIndirect = false;
   const bool AllowHigherAlign = true;
-  // Would rather not naturally align values
-  // Splitting {char, short} into two separate arguments makes that difficult.
+  // Would rather not naturally align values, e.g. passing double on 4 bytes
+  // Splitting {char, short} into two separate arguments makes that difficult
+  // TODO: See if forcing exactly four byte alignment on everything, struct or
+  // scalar, lowers correctly regardless of structs being split across arguments
   return emitVoidPtrVAArg(CGF, VAListAddr, Ty, IsIndirect,
                           getContext().getTypeInfoInChars(Ty),
                           CharUnits::fromQuantity(1), AllowHigherAlign);

llvm/include/llvm/Transforms/IPO/ExpandVariadics.h

@@ -17,10 +17,10 @@ class ModulePass;
 class OptimizationLevel;
 
 enum class ExpandVariadicsMode {
-  unspecified,
-  disable,
-  optimize,
-  lowering,
+  Unspecified, // Use the implementation defaults
+  Disable, // Disable the pass entirely
+  Optimize, // Optimise without changing ABI
+  Lowering, // Change variadic calling convention
 };
 
 class ExpandVariadicsPass : public PassInfoMixin<ExpandVariadicsPass> {

llvm/lib/Target/AMDGPU/AMDGPUPassRegistry.def

@@ -24,7 +24,7 @@ MODULE_PASS("amdgpu-lower-ctor-dtor", AMDGPUCtorDtorLoweringPass())
 MODULE_PASS("amdgpu-lower-module-lds", AMDGPULowerModuleLDSPass(*this))
 MODULE_PASS("amdgpu-printf-runtime-binding", AMDGPUPrintfRuntimeBindingPass())
 MODULE_PASS("amdgpu-unify-metadata", AMDGPUUnifyMetadataPass())
-MODULE_PASS("expand-variadics", ExpandVariadicsPass(ExpandVariadicsMode::lowering))
+MODULE_PASS("expand-variadics", ExpandVariadicsPass(ExpandVariadicsMode::Lowering))
 #undef MODULE_PASS
 
 #ifndef FUNCTION_PASS

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

@@ -975,7 +975,7 @@ void AMDGPUPassConfig::addIRPasses() {
   if (isPassEnabled(EnableImageIntrinsicOptimizer))
     addPass(createAMDGPUImageIntrinsicOptimizerPass(&TM));
 
-  addPass(createExpandVariadicsPass(ExpandVariadicsMode::lowering));
+  addPass(createExpandVariadicsPass(ExpandVariadicsMode::Lowering));
 
   // Function calls are not supported, so make sure we inline everything.
   addPass(createAMDGPUAlwaysInlinePass());

llvm/lib/Target/NVPTX/NVPTXPassRegistry.def

@@ -18,6 +18,7 @@
 #endif
 MODULE_PASS("generic-to-nvvm", GenericToNVVMPass())
 MODULE_PASS("nvptx-lower-ctor-dtor", NVPTXCtorDtorLoweringPass())
+MODULE_PASS("expand-variadics", ExpandVariadicsPass(ExpandVariadicsMode::Lowering))
 #undef MODULE_PASS
 
 #ifndef FUNCTION_ANALYSIS

llvm/lib/Target/NVPTX/NVPTXTargetMachine.cpp

@@ -325,7 +325,7 @@ void NVPTXPassConfig::addIRPasses() {
   }));
 
   // Should run before anything (else!) that adjusts calling conventions
-  addPass(createExpandVariadicsPass(ExpandVariadicsMode::lowering));
+  addPass(createExpandVariadicsPass(ExpandVariadicsMode::Lowering));
 
   // NVVMReflectPass is added in addEarlyAsPossiblePasses, so hopefully running
   // it here does nothing.  But since we need it for correctness when lowering

llvm/lib/Transforms/IPO/ExpandVariadics.cpp

@@ -53,26 +53,27 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/TargetParser/Triple.h"
 
-#include <cstdio>
-
 #define DEBUG_TYPE "expand-variadics"
 
 using namespace llvm;
 
 cl::opt<ExpandVariadicsMode> ExpandVariadicsModeOption(
     DEBUG_TYPE "-override", cl::desc("Override the behaviour of " DEBUG_TYPE),
-    cl::init(ExpandVariadicsMode::unspecified),
-    cl::values(clEnumValN(ExpandVariadicsMode::unspecified, "unspecified",
+    cl::init(ExpandVariadicsMode::Unspecified),
+    cl::values(clEnumValN(ExpandVariadicsMode::Unspecified, "unspecified",
                           "Use the implementation defaults"),
-               clEnumValN(ExpandVariadicsMode::disable, "disable",
+               clEnumValN(ExpandVariadicsMode::Disable, "disable",
                           "Disable the pass entirely"),
-               clEnumValN(ExpandVariadicsMode::optimize, "optimize",
+               clEnumValN(ExpandVariadicsMode::Optimize, "optimize",
                           "Optimise without changing ABI"),
-               clEnumValN(ExpandVariadicsMode::lowering, "lowering",
+               clEnumValN(ExpandVariadicsMode::Lowering, "lowering",
                           "Change variadic calling convention")));
 
 namespace {
 
+// At present Intrinsic:: has no interface to test if a declaration is in the
+// module without creating one. Inserting a declaration and then testing if it
+// has any uses and then deleting it seems a bad way to do the query.
 // Module implements getFunction() which returns nullptr on missing declaration
 // and getOrInsertFunction which creates one when absent. Intrinsics.h
 // implements getDeclaration which creates one when missing. This should be
@@ -319,7 +320,7 @@ class ExpandVariadics : public ModulePass {
   static ExpandVariadicsMode
   withCommandLineOverride(ExpandVariadicsMode LLVMRequested) {
     ExpandVariadicsMode UserRequested = ExpandVariadicsModeOption;
-    return (UserRequested == ExpandVariadicsMode::unspecified) ? LLVMRequested
+    return (UserRequested == ExpandVariadicsMode::Unspecified) ? LLVMRequested
                                                                : UserRequested;
   }
 
@@ -346,7 +347,7 @@ class ExpandVariadics : public ModulePass {
   // Entry point
   bool runOnModule(Module &M) override;
 
-  bool rewriteABI() { return Mode == ExpandVariadicsMode::lowering; }
+  bool rewriteABI() { return Mode == ExpandVariadicsMode::Lowering; }
 
   void memcpyVAListPointers(const DataLayout &DL, IRBuilder<> &Builder,
                             Value *Dst, Value *Src) {
@@ -550,7 +551,7 @@ class ExpandVariadics : public ModulePass {
 
 bool ExpandVariadics::runOnModule(Module &M) {
   bool Changed = false;
-  if (Mode == ExpandVariadicsMode::disable)
+  if (Mode == ExpandVariadicsMode::Disable)
     return Changed;
 
   llvm::Triple Triple(M.getTargetTriple());
@@ -562,7 +563,7 @@ bool ExpandVariadics::runOnModule(Module &M) {
 
   ABI = VariadicABIInfo::create(Triple);
   if (!ABI) {
-    if (Mode == ExpandVariadicsMode::lowering) {
+    if (Mode == ExpandVariadicsMode::Lowering) {
       report_fatal_error(
           "Requested variadic lowering is unimplemented on this target");
     }
@@ -608,7 +609,7 @@ bool ExpandVariadics::runOnModule(Module &M) {
   // and va_copy are removed. All that remains is for the lowering pass to find
   // indirect calls and rewrite those as well.
 
-  if (Mode == ExpandVariadicsMode::lowering) {
+  if (Mode == ExpandVariadicsMode::Lowering) {
     for (Function &F : llvm::make_early_inc_range(M)) {
       if (F.isDeclaration())
         continue;
@@ -638,8 +639,6 @@ bool ExpandVariadics::runOnFunction(Module &M, IRBuilder<> &Builder,
                                     Function *F) {
   bool Changed = false;
 
-  // fprintf(stderr, "Called runOn: %s\n", F->getName().str().c_str());
-
   // This check might be too coarse - there are probably cases where
   // splitting a function is bad but it's usable without splitting
   if (!expansionApplicableToFunction(M, F))
@@ -674,7 +673,7 @@ bool ExpandVariadics::runOnFunction(Module &M, IRBuilder<> &Builder,
         Value *calledOperand = CB->getCalledOperand();
         if (F == calledOperand) {
           report_fatal_error(
-              "ExpandVA abi requires eliminating call uses first\n");
+              "ExpandVA abi requires eliminating call uses first");
         }
       }
 
@@ -1049,8 +1048,8 @@ PreservedAnalyses ExpandVariadicsPass::run(Module &M, ModuleAnalysisManager &) {
 
 ExpandVariadicsPass::ExpandVariadicsPass(OptimizationLevel Level)
     : ExpandVariadicsPass(Level == OptimizationLevel::O0
-                              ? ExpandVariadicsMode::disable
-                              : ExpandVariadicsMode::optimize) {}
+                              ? ExpandVariadicsMode::Disable
+                              : ExpandVariadicsMode::Optimize) {}
 
 ExpandVariadicsPass::ExpandVariadicsPass(ExpandVariadicsMode Mode)
     : ConstructedMode(Mode) {}