[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 ↗

(On Diff #52234)

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 ↗

(On Diff #52234)

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 ↗

(On Diff #52234)

SGTM

[filcab]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1837 ↗

(On Diff #52234)

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

llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp

Value *LibCallSimplifier::optimizePrintFString(CallInst *CI, IRBuilder<> &B) {
 
   // 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);
-  }
+  if (FormatStr.size() == 1)
+    return emitPutChar(B.getInt32(FormatStr[0]), B, TLI);
 
   // 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);
+    return emitPutChar(B.getInt32(ChrStr[0]), B, TLI);
   }
 
   // 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);
+    return emitPutS(GV, B, TLI);
   }
 
   // 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);
-  }
+      CI->getArgOperand(1)->getType()->isIntegerTy())
+    return emitPutChar(CI->getArgOperand(1), B, TLI);
 
   // printf("%s\n", str) --> puts(str)
   if (FormatStr == "%s\n" && CI->getNumArgOperands() > 1 &&
-      CI->getArgOperand(1)->getType()->isPointerTy()) {
+      CI->getArgOperand(1)->getType()->isPointerTy())
     return emitPutS(CI->getArgOperand(1), B, TLI);
-  }
   return nullptr;
 }
 
 Value *LibCallSimplifier::optimizePrintF(CallInst *CI, IRBuilder<> &B) {
 
   Function *Callee = CI->getCalledFunction();
   // Require one fixed pointer argument and an integer/void result.
   FunctionType *FT = Callee->getFunctionType();