Page MenuHomePhabricator

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

Authored by davide on Mar 23 2016, 6:03 PM.

Details

Reviewers
spatel
majnemer
Summary

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

Diff Detail

Event Timeline

davide updated this revision to Diff 51500.Mar 23 2016, 6:03 PM
davide retitled this revision from to [SimplifyLibCalls] Transform printf("%s", "a") into putchar('a').
davide updated this object.
davide added reviewers: majnemer, spatel.
davide added a subscriber: llvm-commits.
spatel added inline comments.Mar 24 2016, 8:34 AM
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 added inline comments.Mar 27 2016, 6:38 PM
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 updated this revision to Diff 51766.Mar 27 2016, 6:51 PM

Sanjay's comments.

spatel added inline comments.Mar 28 2016, 8:34 AM
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 added inline comments.Mar 28 2016, 8:48 AM
lib/Transforms/Utils/SimplifyLibCalls.cpp
1852

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

spatel accepted this revision.Mar 28 2016, 8:55 AM
spatel edited edge metadata.

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.

This revision is now accepted and ready to land.Mar 28 2016, 8:55 AM
davide closed this revision.Mar 28 2016, 9:00 AM

Committed (with the comment) in r264588. Thanks!