This is an archive of the discontinued LLVM Phabricator instance.

[SLC] Convert some strndup calls to strdup calls
ClosedPublic

Authored by xbolva00 on Sep 17 2019, 1:42 PM.

Details

Summary

Motivation:

  • If we can fold it to strdup, we should (strndup does more things than strdup).
  • Annotation mechanism. (Works for strdup well).

strdup and strndup are part of C 20 (currently posix fns), so we should optimize them.

Diff Detail

Event Timeline

xbolva00 created this revision.Sep 17 2019, 1:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2019, 1:42 PM
xbolva00 updated this revision to Diff 220569.Sep 17 2019, 1:43 PM
xbolva00 retitled this revision from [SLC] Convert some strdup calls to strndup calls to [SLC] Convert some strndup calls to strdup calls.
xbolva00 added a reviewer: efriedma.
xbolva00 added a reviewer: jdoerfert.
xbolva00 updated this revision to Diff 220572.Sep 17 2019, 1:47 PM

Removed useless condition

xbolva00 marked an inline comment as done.Sep 17 2019, 3:06 PM
xbolva00 added inline comments.
lib/Transforms/InstCombine/InstCombineCalls.cpp
4226

sight, this breaks things (eg. zstd build)

jdoerfert added inline comments.Sep 17 2019, 7:53 PM
lib/Transforms/InstCombine/InstCombineCalls.cpp
4226

Why ;) ?

test/Transforms/InstCombine/strndup.ll
21

Why don't we get a dereferenceable_or_null(4) here?

xbolva00 marked an inline comment as done.Sep 17 2019, 10:31 PM
xbolva00 added inline comments.
test/Transforms/InstCombine/strndup.ll
21

annotateAllocSite doesnt know about strndup yet :) follow-up patch will solve it.

optimizeStrNDup seems fine to me, the breakage that you mentioned need to be addressed though.

xbolva00 updated this revision to Diff 220663.Sep 18 2019, 7:31 AM

Fixed crash

xbolva00 marked an inline comment as done.Sep 18 2019, 7:49 AM
xbolva00 added inline comments.
lib/Transforms/InstCombine/InstCombineCalls.cpp
4226

This function (visitAllocSite) is just weird. Looking at eraseInstFromFunction, eraseInstFromFunction always returns nullptr; so visitAllocSite returns nullptr in both branches too. If I changed it, it breaks the compiler on some tests.

Instead of hacking this, if think this new patch is reasonable. First, it tries to optimize call, and then possibly visit (remove/annotate) AllocSite.

I think some patches just severely lack words.
It is entirely non-obvious to me why this fold should happen, yet the patch description is empty.

Multiple reasons

  1. If we can fold it to strdup, we should (strndup does more things than strdup).
  2. Annotation mechanism. (Works for strdup well).

p.s: strdup and strndup are part of C 20 :)

Multiple reasons

  1. If we can fold it to strdup, we should (strndup does more things than strdup).
  2. Annotation mechanism. (Works for strdup well).

p.s: strdup and strndup are part of C 20 :)

Put this in the commit message. I missed that it was empty, thanks @lebedev.ri.

xbolva00 edited the summary of this revision. (Show Details)Sep 21 2019, 3:29 AM

Added

@jdoerfert is this patch fine for you?

This revision is now accepted and ready to land.Sep 23 2019, 5:13 AM
This revision was automatically updated to reflect the committed changes.