[SimplifyLibCalls] Strip dead code in printf() transformations

[davide]

Summary

If the return value of printf() is used, we don't do any transformation in SimplifyLibCalls.
Yet, in each transformation we check again if Ci->use_empty(). I think this check is redundant, because it's always true. This also allows to strip some code. IT passes the testsuite, but given I wasn't entirely sure of what are the effects, I put an assert() instead.

[spatel]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1828–1832

IMO, the asserts are unnecessary since the check for empty is right here and has a nice explanatory comment too.

If others like the added safety of the asserts, I don't object. There is a proposal to refactor this whole file, so maybe asserts will be handy.

But instead of repeating the assert 5x, add a one-line helper function or lambda?

[davide]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1828–1832

My vote is to remove the asserts alltogheter. David?

[spatel]

My vote is to remove the assertions; if you do that, then LGTM.
Up to you if you'd like to wait for David to review.

[majnemer]

LGTM

lib/Transforms/Utils/SimplifyLibCalls.cpp

1828–1832

SGTM

[filcab]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1837

What about the !Res case? If the target has printf, but not putchar, for some reason? Or do we simply not care about those (which I think is valid :) )?

[davide]

I assume that might be a problem.
This change is NFC because the first expression always evaluate to true so we never care about the !Res case even prior to this change. In case we hit a case you describe I'll be more than happy to fix but I think the possibility of an environment defining printf() and not putchar() is very remote anyway.

[filcab]

Do you want to add an assert somewhere?
Just in case :-)

Thank you,

Filipe

[spatel]

In D18656#390825, @filcab wrote:

Do you want to add an assert somewhere?
Just in case :-)

I think we're already safe here. On failure (eg, putchar is not available), emitPutChar() returns nullptr, so optimizePrintFString() will also return nullptr to signal to its caller that the transform didn't work.

[filcab]

Right.

Thank you,

Filipe

lib/Transforms/Utils/SimplifyLibCalls.cpp

   StringRef FormatStr;
   if (!getConstantStringInfo(CI->getArgOperand(0), FormatStr))
     return nullptr;
 
   // Empty format string -> noop.
   if (FormatStr.empty()) // Tolerate printf's declared void.
     return CI->use_empty() ? (Value *)CI : ConstantInt::get(CI->getType(), 0);
 
   // 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;
 
   // 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);
+    assert(CI->use_empty() &&
+           "printf return value used should inhibit optimizations");
+    return Res;
   }
 
   // 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);
-
-    // FIXME: Here we check that the return value is not used
-    // but ealier we prevent transformations in case it is.
-    // This should probably be an assert.
-    if (CI->use_empty() || !Res)
-      return Res;
-
-    return B.CreateIntCast(Res, CI->getType(), true);
+    assert(CI->use_empty() &&
+           "printf return value used should inhibit optimizations");
+    return Res;
   }
 
   // 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);
-    return (CI->use_empty() || !NewCI)
-               ? NewCI
-               : ConstantInt::get(CI->getType(), FormatStr.size() + 1);
+    assert(CI->use_empty() &&
+           "printf return value used should inhibit optimizations");
+    return NewCI;
   }
 
   // Optimize specific format strings.
   // printf("%c", chr) --> putchar(chr)
   if (FormatStr == "%c" && CI->getNumArgOperands() > 1 &&
       CI->getArgOperand(1)->getType()->isIntegerTy()) {
     Value *Res = emitPutChar(CI->getArgOperand(1), B, TLI);
-
-    if (CI->use_empty() || !Res)
-      return Res;
-    return B.CreateIntCast(Res, CI->getType(), true);
+    assert(CI->use_empty() &&
+           "printf return value used should inhibit optimizations");
   }
 
   // printf("%s\n", str) --> puts(str)
   if (FormatStr == "%s\n" && CI->getNumArgOperands() > 1 &&
       CI->getArgOperand(1)->getType()->isPointerTy()) {
-    return emitPutS(CI->getArgOperand(1), B, TLI);
+    Value *Res = emitPutS(CI->getArgOperand(1), B, TLI);
+    assert(CI->use_empty() &&
+           "printf return value used should inhibit optimizations");
+    return Res;
   }
   return nullptr;
 }
 
 Value *LibCallSimplifier::optimizePrintF(CallInst *CI, IRBuilder<> &B) {
 
   Function *Callee = CI->getCalledFunction();
   // Require one fixed pointer argument and an integer/void result.