[PATCH] D100724: [SimplifyLibCalls] Transform printf("%s", str) to puts(str) or noop.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2387
+    if (OperandStr.empty())
+      return (Value *)CI;
+    // printf("%s", "a") --> putchar('a')
----------------
We have seen problems before with this type of change, but I don't remember exactly how we fixed it. 
@xbolva00 may recall. 
Should we use eraseFromParent() to make sure that InstCombine's worklist is updated as expected?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2379
+  // Try to emit putchar or puts.
   if (FormatStr == "%s" && CI->getNumArgOperands() > 1) {
+    StringRef OperandStr;
----------------
yurai007 wrote:
> yurai007 wrote:
> > spatel wrote:
> > > Why is this checking for `args > 1` instead of exactly `args == 2`?
> > That's interesting. It's part of legacy and I think it's deliberate to handle (pathological but accepted by frontend) cases 
> > when printf has redundant arguments and we still want to emit puts or putchar: https://godbolt.org/z/xjxG98jKh
> > Notice that thanks to this condition llvm is more aggresive then gcc. I don't have strong opinion about such behaviour but I would vote for leaving it as it is. Maybe in such case it would be worth to add one more test to catch and document this behaviour.
> test_simplify10 demonstrates mentioned behavior. 
Thanks for explaining and the extra test to check it!


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2397
   // printf("foo\n") --> puts("foo")
-  if (FormatStr[FormatStr.size() - 1] == '\n' &&
+  if (FormatStr.back() == '\n' &&
       FormatStr.find('%') == StringRef::npos) { // No format characters.
----------------
spatel wrote:
> That diff is independent of this patch, so could be an NFC preliminary cleanup.
To be clear, I think using "back()" is an improvement, but it is independent of the functional changes we are making in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100724