[PATCH] D18423: [SimplifyLibCalls] Transform printf("%s", "a") into putchar('a')

[Davide Italiano via llvm-commits]

davide added inline comments.

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1843
@@ +1842,3 @@
+  // printf("%s", "a") --> putchar('a')
+  if (FormatStr == "%s" && CI->getNumArgOperands() > 1) {
+    StringRef ChrStr;
----------------
spatel wrote:
> The number of arguments should be an exact comparison? 
> CI->getNumArgOperands() == 2
Hmm, I don't think so.
Imagine you have:

printf("%c", 'a', 0, 0, 0, 0)
you might still want to lower printf() -> putchar() given the other arguments are unused.

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1849
@@ +1848,3 @@
+      return nullptr;
+    Value *Res = emitPutChar(B.getInt32(FormatStr[0]), B, TLI);
+    if (CI->use_empty() || !Res)
----------------
spatel wrote:
> FormatStr[0] --> ChrStr[0] ?
Nice catch, will fix.

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1852
@@ +1851,3 @@
+      return Res;
+    return B.CreateIntCast(Res, CI->getType(), true);
+  }
----------------
spatel wrote:
> This does not behave the way that we need.
> printf() returns the number of characters that were successfully printed or an error code.
> putchar() returns the character value that was printed or an error code.
> So on success, the transformed call must return '1'. On error, some translation of the error code may be needed:
> http://www.cplusplus.com/reference/cstdio/printf/
> http://www.cplusplus.com/reference/cstdio/putchar/
I was scared, because basically every function violates what you describe.
Then I took a closer look at the function and noticed we have a guard before transformations are attempted.

```  
  // Do not do any of the following transformations if the printf return value
  // is used, in general the printf return value is not compatible with either
  // putchar() or puts().
  if (!CI->use_empty())
    return nullptr;
```

So, while in the future we *might* relax this, I think the transformation as is it's safe for now.

================
Comment at: test/Transforms/InstCombine/printf-2.ll:13
@@ -10,3 +12,3 @@
 
 declare void @printf(i8*, ...)
 
----------------
spatel wrote:
> Please add/use test cases that verify that the return value of printf is handled correctly by this transform.
Already covered (see comment above)


http://reviews.llvm.org/D18423