sameerds added inline comments.
================ Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:212 +// helper struct to package the string related data +typedef struct S { + std::string Str; ---------------- you can just say "struct StringData" ================ Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:218-219 + + S(std::string str = "", bool IC = true, Value *RS = nullptr, + Value *AS = nullptr) + : Str(str), isConst(IC), RealSize(RS), AlignedSize(AS) {} ---------------- Every argument has a default value. Why not simply assign them as member initializers? ================ Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:238 + + // First 8 bytes to be reserved for control dword + size_t BufSize = 4; ---------------- Should this say 4? ================ Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:256 + + StringRef ArgStr; + for (size_t i = 1; i < Args.size(); i++) { ---------------- move this closer to first use ================ Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:313 + SparseBitVector<8> &SpecIsCString, + SmallVector<StringData, 8> &StringContents, + bool isConstFmtStr) { ---------------- function argument should use SmallVectorImpl, and not SmallVector ================ Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:321 + SmallVector<Value *, 32> WhatToStore; + if ((i == 0) || SpecIsCString.test(i)) { + if ((*StrIt).isConst) { ---------------- This "if (...) { ... }" has a really long body. Can it be moved into a separate function for readability? ================ Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:324 + Str = (*StrIt).Str; + const uint64_t ReadSize = 4; + ---------------- Try to move declarations to the smallest scope that they are used in. For ReadSize, this seems to be the while loop later. ================ Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:352 + + // TODO: Should not bothering aligning up. + if (ReadNow < ReadSize) ---------------- typo: bother ================ Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:426-427 + + // The buffered version still follows OpenCL printf standards for + // printf return value, i.e 0 on success, 1 on failure. + ConstantPointerNull *zeroIntPtr = ---------------- So we cannot use a buffered printf in HIP? ================ Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:451 + {ArgSize, + ConstantInt::get(Int1Ty, !FmtStr.empty() ? 1 : 0, false), + ConstantInt::get(Int1Ty, 0, false)}); ---------------- FmtStr.empty() gets checked in multiple places. Can this be captured as one or more boolean variables with meaningful names? ================ Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:487 + if(metaD->getNumOperands() == 0) { + MDString *fmtStrArray = MDString::get(Ctx, "0:0:deadbeef,\"\""); + MDNode *myMD = MDNode::get(Ctx, fmtStrArray); ---------------- Probably not a good choice if it shows up where the user can see it. It's also not very informative. A more descriptive string will be better. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150427/new/ https://reviews.llvm.org/D150427 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits