Page MenuHomePhabricator

[SLC] Convert some strndup calls to strdup calls
ClosedPublic

Authored by xbolva00 on Tue, Sep 17, 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

Repository
rL LLVM

Event Timeline

xbolva00 created this revision.Tue, Sep 17, 1:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 17, 1:42 PM
xbolva00 updated this revision to Diff 220569.Tue, Sep 17, 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.Tue, Sep 17, 1:47 PM

Removed useless condition

xbolva00 marked an inline comment as done.Tue, Sep 17, 3:06 PM
xbolva00 added inline comments.
lib/Transforms/InstCombine/InstCombineCalls.cpp
4226 ↗(On Diff #220572)

sight, this breaks things (eg. zstd build)

jdoerfert added inline comments.Tue, Sep 17, 7:53 PM
lib/Transforms/InstCombine/InstCombineCalls.cpp
4226 ↗(On Diff #220572)

Why ;) ?

test/Transforms/InstCombine/strndup.ll
21 ↗(On Diff #220572)

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

xbolva00 marked an inline comment as done.Tue, Sep 17, 10:31 PM
xbolva00 added inline comments.
test/Transforms/InstCombine/strndup.ll
21 ↗(On Diff #220572)

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.Wed, Sep 18, 7:31 AM

Fixed crash

xbolva00 marked an inline comment as done.Wed, Sep 18, 7:49 AM
xbolva00 added inline comments.
lib/Transforms/InstCombine/InstCombineCalls.cpp
4226 ↗(On Diff #220572)

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)Sat, Sep 21, 3:29 AM

Added

@jdoerfert is this patch fine for you?

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