sameerds added inline comments.
================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:184-185
// specify a string, i.e, the "%s" specifier with optional '*' characters.
-static void locateCStrings(SparseBitVector<8> &BV, Value *Fmt) {
- StringRef Str;
- if (!getConstantStringInfo(Fmt, Str) || Str.empty())
+static void locateCStrings(SparseBitVector<8> &BV, Value *Fmt, StringRef
&FmtStr) {
+ if (!getConstantStringInfo(Fmt, FmtStr) || FmtStr.empty())
return;
----------------
Seems like the only purpose of this change is to make the new StringRef
available outside. It will be less confusing to just move the call to
getConstantStringInfo() out of this function, and pass the StringRef as an
input parameter instead of output. Also, try not to change existing variable
names unless it is really relevant. That reduces the number of lines affected
by mostly non-functional changes.
================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:223
+
+static size_t alignUp(size_t Value, uint alignment) {
+ return (Value + alignment - 1) & ~(alignment - 1);
----------------
Needs consistent capitalization in the argument names. Since the rest of the
code seems to prefer TitleCase, maybe alignment should start with "A"?
================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:369
+ // to alignment padding is not populated with any specific value
+ // here, I feel this would be safe as long as runtime is sync with
+ // the offsets.
----------------
Avoid first person comments.
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
