[PATCH] D158246: [amdgpu] WIP variadics

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:38
+
+#include <cstdio>
+
----------------
Don't need


================
Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:44-47
+static cl::opt<bool>
+    ApplyToAllOverride(DEBUG_TYPE "-all", cl::init(false),
+                       cl::desc("Expand VA intrinsics in all functions"),
+                       cl::Hidden);
----------------
Don't understand the point of this


================
Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:151
+  bool canTransformFunctionInIsolation(Function &F) {
+    if (!F.isVarArg() || F.isDeclaration() || !F.hasLocalLinkage() ||
+        F.hasAddressTaken() || F.hasFnAttribute(Attribute::Naked)) {
----------------
isDefinitionExact?

If this is just how the ABI is going to lower, I don't see why you need to worry about any such restrictions


================
Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:181
+
+      bool isByVal = CB->paramHasAttr(I, Attribute::ByVal);
+      if (isByVal)
----------------
Probably should defend against sret and the other ABI attributes


================
Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:209
+    StructType *VarargsTy = StructType::create(
+        Ctx, LocalVarTypes, (Twine(NF->getName()) + ".vararg").str());
+
----------------
How is StructType::create not using Twine


================
Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:215-217
+    auto alloced = Builder.Insert(
+        new AllocaInst(VarargsTy, DL.getAllocaAddrSpace(), nullptr,
+                       std::max(MaxFieldAlign, assumedStructAlignment(DL))),
----------------
What's wrong with just Builder.CreateAlloca?


================
Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:217
+        new AllocaInst(VarargsTy, DL.getAllocaAddrSpace(), nullptr,
+                       std::max(MaxFieldAlign, assumedStructAlignment(DL))),
+        "vararg_buffer");
----------------
what's wrong with the abi type alignment? why does the stack alignment matter?


================
Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:226
+      Builder.CreateStore(Varargs[i].first,
+                          r); // alignment info could be better
+    }
----------------
alignTo shouldn't be difficult


================
Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:229-230
+
+    Args.push_back(Builder.CreatePointerBitCastOrAddrSpaceCast(
+        alloced, Type::getInt8PtrTy(Ctx)));
+
----------------
just CreateAddrSpaceCast should work?


================
Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:253
+          cast<CallInst>(CB)->getTailCallKind());
+    }
+
----------------
there are more CallBase types than call and invoke now. Can't you just mutate the call operand in place?


================
Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:258
+    NewCB->setCallingConv(CB->getCallingConv());
+    NewCB->copyMetadata(*CB, {LLVMContext::MD_prof, LLVMContext::MD_dbg});
+
----------------
I'd assume all metadata would be preservable if this is just how the ABI works


================
Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:272-276
+    if (knownNaturalStackAlignment) {
+      return DL.getStackAlignment();
+    } else {
+      return {};
+    }
----------------
no return after else, 


================
Comment at: llvm/test/CodeGen/Generic/expand-variadic-intrinsics.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -expand-va-intrinsics -expand-va-intrinsics-all=true -S < %s | FileCheck %s
+
----------------
-passes.

Also "generic" codegen tests are a fiction, use a real triple


================
Comment at: llvm/test/CodeGen/Generic/expand-variadic-intrinsics.ll:48
+  call void @llvm.va_start(ptr nonnull %va)
+  %0 = va_arg ptr %va, i32
+  %1 = va_arg ptr %va, double
----------------
Use named values


================
Comment at: llvm/test/CodeGen/Generic/expand-variadic-intrinsics.ll:76
+}
+
+
----------------
Needs some indirect variadic call tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158246/new/

https://reviews.llvm.org/D158246