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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits