arsenm requested changes to this revision.
arsenm added a comment.
Herald added a subscriber: wdng.
I have a few questions. First, why surface this to users? If we really need to,
I don't think this is the right flag name/design. A named argument to some kind
of printf lowering flag would be better. Second, I thought we were moving
towards hostcall printf, not away from it.
================
Comment at: clang/include/clang/Basic/LangOptions.def:275
LANGOPT(OffloadingNewDriver, 1, 0, "use the new driver for generating
offloading code.")
+LANGOPT(DelayedPrintf, 1, 0, "version onf printf function to be used, hostcall
or buffer based")
----------------
Typo 'onf'
================
Comment at: clang/include/clang/Driver/Options.td:985
+def fdelayed_printf : Flag<["-"], "fdelayed-printf">,
+ HelpText<"Specifies which version of printf is to be used while CodeGen">,
+ Flags<[CC1Option]>,
----------------
Help text reads weird
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4655-4667
+
+ if (JA.isDeviceOffloading(Action::OFK_HIP)) {
+ // Device side compilation printf
+ if (Args.getLastArg(options::OPT_fdelayed_printf))
+ CmdArgs.push_back("-fdelayed-printf");
+ }
}
----------------
Missing clang side tests
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:135-136
-bool AMDGPUPrintfRuntimeBindingImpl::shouldPrintAsStr(char Specifier,
- Type *OpType) const {
- if (Specifier != 's')
- return false;
- const PointerType *PT = dyn_cast<PointerType>(OpType);
- if (!PT || PT->getAddressSpace() != AMDGPUAS::CONSTANT_ADDRESS)
- return false;
- Type *ElemType = PT->getContainedType(0);
- if (ElemType->getTypeID() != Type::IntegerTyID)
- return false;
- IntegerType *ElemIType = cast<IntegerType>(ElemType);
- return ElemIType->getBitWidth() == 8;
+// This function is essentially a copy from the file
+// Transforms/Utils/AMDGPUEmitPrintf.cpp
+static Value *getStrlenWithNull(IRBuilder<> &Builder, Value *Str) {
----------------
Why not share these? They should be in the same place. This should also be a
separate change from any flag changes
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:141
+
+ auto CharZero = Builder.getInt8(0);
+ auto One = Builder.getInt64(1);
----------------
auto *
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:150
+ BasicBlock *Join = nullptr;
+ if (Prev->getTerminator()) {
+ Join = Prev->splitBasicBlock(Builder.GetInsertPoint(), "strlen.join");
----------------
Why would the block be missing a terminator here?
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:183-185
+ auto Begin = Builder.CreatePtrToInt(Str, Int64Ty);
+ auto End = Builder.CreatePtrToInt(PtrPhi, Int64Ty);
+ auto Len = Builder.CreateSub(End, Begin);
----------------
Really should try hard to avoid introducing ptrtoint. Do you really need to do
a pointer subtract?
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:215
+ StringRef Str;
+ llvm::Value *RealSize;
+ llvm::Value *AlignedSize;
----------------
Don't need llvm::
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:439
- Type *idPointer = PointerType::get(I32Ty, AMDGPUAS::GLOBAL_ADDRESS);
+ Type *idPointer = PointerType::get(Int32Ty, AMDGPUAS::GLOBAL_ADDRESS);
Value *id_gep_cast =
----------------
Broken for opaque pointers?
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:443
- new StoreInst(ConstantInt::get(I32Ty, UniqID), id_gep_cast, Brnch);
+ new StoreInst(ConstantInt::get(Int32Ty, UniqID), id_gep_cast, Brnch);
----------------
Why isn't this using the IRBuilder?
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:491-492
+ if (S[0]) {
+ char *MyNewStr = new char[NSizeStr]();
+ strcpy(MyNewStr, S.str().c_str());
+ int NumInts = NSizeStr / 4;
----------------
Why do we have raw new and strcpy here? We have better string classes
================
Comment at: llvm/test/CodeGen/AMDGPU/hip-delayed-printf.ll:1
+; RUN: opt -mtriple=amdgcn--amdhsa -passes=amdgpu-printf-runtime-binding -S <
%s | FileCheck --check-prefix=FUNC --check-prefix=GCN --check-prefix=METADATA %s
+
----------------
Don't need these separate prefixes
================
Comment at: llvm/test/CodeGen/AMDGPU/hip-delayed-printf.ll:3
+
+; FUNC-LABEL: @test_kernel(
+; GCN-LABEL: entry
----------------
Should use generated checks
================
Comment at: llvm/test/CodeGen/AMDGPU/hip-delayed-printf.ll:39
+ store i32 25, ptr %p.ascast, align 4
+ %0 = load i32, ptr %p.ascast, align 4
+ %cmp = icmp sgt i32 %0, 30
----------------
Use named values in tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138702/new/
https://reviews.llvm.org/D138702
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
