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

[davide]

Summary

We were missing this case. GCC optimizes it, apparently.

[spatel]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1843

The number of arguments should be an exact comparison?
CI->getNumArgOperands() == 2

1849

FormatStr[0] --> ChrStr[0] ?

1852

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/

test/Transforms/InstCombine/printf-2.ll

13

Please add/use test cases that verify that the return value of printf is handled correctly by this transform.

[davide]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1843

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.

1849

Nice catch, will fix.

1852

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.

test/Transforms/InstCombine/printf-2.ll

13

Already covered (see comment above)

[davide]

Sanjay's comments.

[spatel]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1843

Ok - clang does warn in that case ( warning: data argument not used by format string [-Wformat-extra-args] ), but the extra args are not removed by the front-end, so this looks fine.

1852

Ah, I didn't look far enough ahead to see that guard.
But if we know that CI->use_empty() is always true at this point, then why are we checking it here? If we want to be safe, we could assert that condition, but I don't think that's necessary given that the guard is just a few lines up in the code. Unless I've misunderstood, we should make that change in all of the related printf transforms too.

test/Transforms/InstCombine/printf-2.ll

13

Ok - there is an existing negative test to check that the transform doesn't fire when the return value is used in llvm/test/Transforms/InstCombine/printf-1.ll.

[davide]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1852

Good idea, maybe in the next commit I can do a global sweep?

[spatel]

Good idea, maybe in the next commit I can do a global sweep?

That would be fine with me. But if you do it that way, please do add a 'FIXME' comment in this patch just in case it doesn't happen immediately.

[davide]

Committed (with the comment) in r264588. Thanks!

lib/Transforms/Utils/SimplifyLibCalls.cpp

   // printf("x") -> putchar('x'), even for '%'.
   if (FormatStr.size() == 1) {
     Value *Res = emitPutChar(B.getInt32(FormatStr[0]), B, TLI);
     if (CI->use_empty() || !Res)
       return Res;
     return B.CreateIntCast(Res, CI->getType(), true);
   }
 
+  // printf("%s", "a") --> putchar('a')
+  if (FormatStr == "%s" && CI->getNumArgOperands() > 1) {
+    StringRef ChrStr;
+    if (!getConstantStringInfo(CI->getOperand(1), ChrStr))
+      return nullptr;
+    if (ChrStr.size() != 1)
+      return nullptr;
+    Value *Res = emitPutChar(B.getInt32(ChrStr[0]), B, TLI);
+    if (CI->use_empty() || !Res)
+      return Res;
+    return B.CreateIntCast(Res, CI->getType(), true);
+  }
+
   // printf("foo\n") --> puts("foo")
   if (FormatStr[FormatStr.size() - 1] == '\n' &&
       FormatStr.find('%') == StringRef::npos) { // No format characters.
     // Create a string literal with no \n on it.  We expect the constant merge
     // pass to be run after this pass, to merge duplicate strings.
     FormatStr = FormatStr.drop_back();
     Value *GV = B.CreateGlobalString(FormatStr, "str");
     Value *NewCI = emitPutS(GV, B, TLI);

test/Transforms/InstCombine/printf-2.ll

 ; Test that the printf library call simplifier works correctly.
 ;
 ; RUN: opt < %s -instcombine -S | FileCheck %s
 
 target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:128:128"
 
 @hello_world = constant [13 x i8] c"hello world\0A\00"
 @h = constant [2 x i8] c"h\00"
 @percent_s = constant [4 x i8] c"%s\0A\00"
+@format_str = constant [3 x i8] c"%s\00"
+@charstr = constant [2 x i8] c"a\00"
 
 declare void @printf(i8*, ...)
 
 ; Check simplification of printf with void return type.
 
 define void @test_simplify1() {
 ; CHECK-LABEL: @test_simplify1(
   %fmt = getelementptr [2 x i8], [2 x i8]* @h, i32 0, i32 0
   call void (i8*, ...) @printf(i8* %fmt)
 ; CHECK-NEXT: call i32 @putchar(i32 104)

 ; CHECK-LABEL: @test_simplify6(
   %fmt = getelementptr [4 x i8], [4 x i8]* @percent_s, i32 0, i32 0
   %str = getelementptr [13 x i8], [13 x i8]* @hello_world, i32 0, i32 0
   call void (i8*, ...) @printf(i8* %fmt, i8* %str)
 ; CHECK-NEXT: call i32 @puts(i8* getelementptr inbounds ([13 x i8], [13 x i8]* @hello_world, i32 0, i32 0))
   ret void
 ; CHECK-NEXT: ret void
 }
+
+define void @test_simplify7() {
+; CHECK-LABEL: @test_simplify7(
+  %fmt = getelementptr [3 x i8], [3 x i8]* @format_str, i32 0, i32 0
+  %str = getelementptr [2 x i8], [2 x i8]* @charstr, i32 0, i32 0
+  call void (i8*, ...) @printf(i8* %fmt, i8* %str)
+; CHECK-NEXT: call i32 @putchar(i32 97)
+  ret void
+; CHECK-NEXT: ret void
+}