Page MenuHomePhabricator

[SLC] sprintf(dst, "%s", str) -> strcpy(dst, str)
ClosedPublic

Authored by xbolva00 on Aug 14 2020, 2:58 AM.

Details

Summary

Transform sprintf(dst, "%s", str) -> strcpy(dst, str) if result is unused
Avoid sprintf(dest, "%s", str) -> llvm.memcpy(align 1 dest, align 1 str, strlen(str)+1) if optimizing for size.

Diff Detail

Event Timeline

xbolva00 created this revision.Aug 14 2020, 2:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2020, 2:58 AM
xbolva00 requested review of this revision.Aug 14 2020, 2:58 AM
xbolva00 edited the summary of this revision. (Show Details)Aug 14 2020, 3:07 AM
xbolva00 retitled this revision from SLC] sprintf(dst, "%s", str) -> strcpy(dst, str) to [SLC] sprintf(dst, "%s", str) -> strcpy(dst, str).Aug 14 2020, 6:34 AM
xbolva00 added a reviewer: spatel.

In the case where we need the return value, there are a couple possible modifications:

  1. Maybe worth checking if the length of the string is a known constant.
  2. We could consider using stpcpy on targets where it's available.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2501

I don't think I've seen this hasOptSize() || shouldOptimizeForSize() pattern before; is there documentation anywhere?

xbolva00 updated this revision to Diff 285668.Aug 14 2020, 9:13 AM

Implemented suggested transformations.

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

Also used eg in “optimizeFputs”

It was introducted as part of PGSO.

In the case where we need the return value, there are a couple possible modifications:

  1. Maybe worth checking if the length of the string is a known constant.
  2. We could consider using stpcpy on targets where it's available.

Thanks, great suggestions;) Please check if correct.

xbolva00 added inline comments.Aug 14 2020, 9:18 AM
llvm/test/Transforms/InstCombine/sprintf-1.ll
169

Should we do this under minsize or not?

Extra sub in asm should be ok, but IR has sub and 2x ptrtoints now.

LGTM

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

This is basically the first place we're generating stpcpy "from scratch". That's probably okay, since gcc does it, but we should watch out for bug reports.

llvm/test/Transforms/InstCombine/sprintf-1.ll
169

The ptrtoints are free at the assembly level; the total number of IR instructions isn't important.

efriedma accepted this revision.Aug 14 2020, 2:34 PM
This revision is now accepted and ready to land.Aug 14 2020, 2:34 PM
This revision was automatically updated to reflect the committed changes.

This change still causes failed asserts when building source like e.g. this:

$ cat sprintf-strcpy.c
char *ptr; void func(void) { ptr += sprintf(ptr, "%s", ""); }
$ clang -c sprintf-strcpy.c -O2 -target x86_64-linux-gnu
clang: ../lib/IR/Value.cpp:473: void llvm::Value::doRAUW(llvm::Value*, llvm::Value::ReplaceMetadataUses): Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!"' failed.

Will revert it shortly.