[SimplifyLibCalls] Transform printf("%s", str) --> puts(str)/noop.

[yurai007]

Summary

Before this change LLVM cannot simplify printf in following cases:

printf("%s", "") --> noop
printf("%s", str"\n") --> puts(str)

From the other hand GCC can perform such transformations for many years:
https://godbolt.org/z/7nnqbedfe

[xbolva00]

Looks fine

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2397

Can you change this to FormatStr.back() ?

[yurai007]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2397

Sure.

[xbolva00]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2397

Thanks, otherwise it is OK from me.

Mayybe @spatel / @fhahn / @efriedma take a second look?

[xbolva00]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2397

I was wondering if we can check content of "str.1" and yes! with --check-globals flag

Can you run ./update_test_checks.py --check-globals /home/xbolva00/p.ll llvm/test/Transforms/InstCombine/printf-2.ll ? It would be ideal to have check for content of str.1 too.

[yurai007]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2397

Ok. Now test has extra strings checking (hope did it correctly). It extends patch scope a little bit so I updated 'summary' and commit message.

[lebedev.ri]

llvm/test/Transforms/InstCombine/printf-2.ll

67

This does't fire when @printf returns the status, and/or when that status is used, right?

[yurai007]

llvm/test/Transforms/InstCombine/printf-2.ll

67

That's right. Currently we run all kinds of printf -> puts/putchar transformations only when printf has no users. OptimizePrintFString checks that at beginning.

[yurai007]

@lebedev.ri @xbolva00: Kindly ping :) All comments were addressed. If there is anything more to do please let me know.

[xbolva00]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2383–2384

We could even handle printf("%s", "");

if (OperandStr.empty())

return ConstantInt::get(CI->getType(), 0);

https://godbolt.org/z/Ke5bcxWPK

[spatel]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2379–2380

Why is this checking for args > 1 instead of exactly args == 2?

2382

What does it mean if the operand string is empty?

printf("%s", "");

It can be another patch, but please leave a "TODO" comment. Looks like gcc removes the call entirely.

2388

Style: remove else ( https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return )

2397

That diff is independent of this patch, so could be an NFC preliminary cleanup.

[yurai007]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2382

Ok, will add it to this patch if you don't mind.

2383–2384

Nice finding. I will include this optimization to patch.

[yurai007]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2379–2380

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.

[yurai007]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2379–2380

test_simplify10 demonstrates mentioned behavior.

[xbolva00]

I commited your NFC patches, you can rebase this patch now.

[spatel]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2379–2380

Thanks for explaining and the extra test to check it!

2386

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?

2397

To be clear, I think using "back()" is an improvement, but it is independent of the functional changes we are making in this patch.

[yurai007]

Rebased on top of NFCs.

[xbolva00]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2386

This pattern is used from line 2368 so probably nothing to worry about.

[spatel]

LGTM - it would be a slight improvement to add the tests first with baseline checks, so we just have the diffs here.

[yurai007]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2386

I think that as long as CI->use_empty() - which is the case - there shouldn't be any problem.

[yurai007]

@spatel: Ok. I will split patch into two.

[yurai007]

Rebased on top of last NFC 05c912a439cc.

[yurai007]

@xbolva00, @spatel: Ping. It's rebased to 8a9fbaa071 and ready to land I think.

[yurai007]

@xbolva00: Thanks for looking into https://reviews.llvm.org/D101176! However my ping was about parallel, accepted change: https://reviews.llvm.org/D100724 which I cannot push myself as I don't have permission.

[spatel]

Is this the correct email address to attribute as patch author?
[e-mail]
https://llvm.org/docs/DeveloperPolicy.html#commit-messages

[yurai007]

@spatel: Yes, that's correct address.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

Value *LibCallSimplifier::optimizePrintFString(CallInst *CI, IRBuilderBase &B) {
   // putchar() or puts().
   if (!CI->use_empty())
     return nullptr;
 
   // printf("x") -> putchar('x'), even for "%" and "%%".
   if (FormatStr.size() == 1 || FormatStr == "%%")
     return emitPutChar(B.getInt32(FormatStr[0]), B, TLI);
 
-  // printf("%s", "a") --> putchar('a')
+  // Try to emit putchar or puts.
   if (FormatStr == "%s" && CI->getNumArgOperands() > 1) {
-    StringRef ChrStr;
-    if (!getConstantStringInfo(CI->getOperand(1), ChrStr))
+    StringRef OperandStr;
+    if (!getConstantStringInfo(CI->getOperand(1), OperandStr) ||
+        OperandStr.empty())
       return nullptr;
-    if (ChrStr.size() != 1)
-      return nullptr;
-    return emitPutChar(B.getInt32(ChrStr[0]), B, TLI);
+    // printf("%s", "a") --> putchar('a')
+    if (OperandStr.size() == 1)
+      return emitPutChar(B.getInt32(OperandStr[0]), B, TLI);
+    // printf("%s", str"\n") --> puts(str)
+    else if (OperandStr.back() == '\n') {
+      OperandStr = OperandStr.drop_back();
+      Value *GV = B.CreateGlobalString(OperandStr, "str");
+      return emitPutS(GV, B, TLI);
+    }
   }
 
   // printf("foo\n") --> puts("foo")
-  if (FormatStr[FormatStr.size() - 1] == '\n' &&
+  if (FormatStr.back() == '\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");
     return emitPutS(GV, B, TLI);
   }
 

llvm/test/Transforms/InstCombine/printf-2.ll

 ; CHECK-NEXT:    [[PUTCHAR:%.*]] = call i32 @putchar(i32 97)
 ; CHECK-NEXT:    ret void
 ;
   %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)
   ret void
 }
+
+define void @test_simplify8() {
+; CHECK-LABEL: @test_simplify8(
+; CHECK-NEXT:    [[PUTS:%.*]] = call i32 @puts(i8* nonnull dereferenceable(1) getelementptr inbounds ([12 x i8], [12 x i8]* @str.1, i32 0, i32 0))
+; CHECK-NEXT:    ret void
+;
+  %fmt = getelementptr [3 x i8], [3 x i8]* @format_str, 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)
+  ret void
+}