Page MenuHomePhabricator

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

Authored by yurai007 on Apr 18 2021, 5:50 AM.

Details

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

Diff Detail

Event Timeline

yurai007 created this revision.Apr 18 2021, 5:50 AM
yurai007 requested review of this revision.Apr 18 2021, 5:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2021, 5:50 AM

Looks fine

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

Can you change this to FormatStr.back() ?

yurai007 updated this revision to Diff 338373.Apr 18 2021, 6:28 AM
yurai007 removed rG LLVM Github Monorepo as the repository for this revision.
yurai007 marked an inline comment as done.Apr 18 2021, 6:33 AM
yurai007 added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2401

Sure.

xbolva00 added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2401

Thanks, otherwise it is OK from me.

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

xbolva00 added inline comments.Apr 18 2021, 7:07 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2401

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 updated this revision to Diff 338380.Apr 18 2021, 8:31 AM
yurai007 marked an inline comment as done.
yurai007 edited the summary of this revision. (Show Details)
yurai007 marked an inline comment as done.Apr 18 2021, 8:34 AM
yurai007 added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2401

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 added inline comments.Apr 18 2021, 8:37 AM
llvm/test/Transforms/InstCombine/printf-2.ll
79

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

yurai007 marked an inline comment as done.Apr 18 2021, 8:51 AM
yurai007 added inline comments.
llvm/test/Transforms/InstCombine/printf-2.ll
79

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 marked 2 inline comments as done.Apr 20 2021, 11:28 PM

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

xbolva00 accepted this revision.Apr 21 2021, 4:46 AM
xbolva00 added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2384–2385

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

if (OperandStr.empty())

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

https://godbolt.org/z/Ke5bcxWPK

This revision is now accepted and ready to land.Apr 21 2021, 4:46 AM
spatel added inline comments.Apr 21 2021, 4:57 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2381–2382

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

2384

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.

2389
2401

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

yurai007 updated this revision to Diff 339538.Apr 22 2021, 2:38 AM
yurai007 edited the summary of this revision. (Show Details)
yurai007 marked an inline comment as done.
yurai007 added inline comments.Apr 22 2021, 2:48 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2384

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

2384–2385

Nice finding. I will include this optimization to patch.

yurai007 added inline comments.Apr 22 2021, 3:29 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2381–2382

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 updated this revision to Diff 339585.Apr 22 2021, 5:35 AM
yurai007 retitled this revision from [SimplifyLibCalls] Transform printf("%s", str"\n") --> puts(str). to [SimplifyLibCalls] Transform printf("%s", str) to puts(str) or noop..
yurai007 edited the summary of this revision. (Show Details)
yurai007 marked 4 inline comments as done.Apr 22 2021, 5:38 AM
yurai007 added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2381–2382

test_simplify10 demonstrates mentioned behavior.

yurai007 edited the summary of this revision. (Show Details)Apr 22 2021, 6:04 AM

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

spatel added inline comments.Apr 22 2021, 6:29 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2381–2382

Thanks for explaining and the extra test to check it!

2387

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?

2401

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 updated this revision to Diff 339604.Apr 22 2021, 6:34 AM

Rebased on top of NFCs.

xbolva00 added inline comments.Apr 22 2021, 6:54 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2387

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

yurai007 marked 2 inline comments as done.Apr 22 2021, 7:14 AM
spatel accepted this revision.Apr 22 2021, 7:16 AM

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

yurai007 marked an inline comment as done.Apr 22 2021, 7:17 AM
yurai007 added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2387

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

yurai007 marked an inline comment as done.Apr 22 2021, 7:18 AM

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

yurai007 retitled this revision from [SimplifyLibCalls] Transform printf("%s", str) to puts(str) or noop. to [SimplifyLibCalls] Transform printf("%s", str) --> puts(str)/noop..Apr 23 2021, 2:44 AM
yurai007 updated this revision to Diff 340038.Apr 23 2021, 8:04 AM

Rebased on top of last NFC 05c912a439cc.

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

@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.

Is this the correct email address to attribute as patch author?
dawid_jurek@vp.pl
https://llvm.org/docs/DeveloperPolicy.html#commit-messages

@spatel: Yes, that's correct address.

This revision was automatically updated to reflect the committed changes.