This is an archive of the discontinued LLVM Phabricator instance.

[DSE] Improve handling of `strncpy` in Dead Store Elimination
ClosedPublic

Authored by fwolff on Nov 16 2021, 2:11 PM.

Details

Summary

Fixes PR#52062 and one of the remaining cases of PR#47644.

Diff Detail

Event Timeline

fwolff created this revision.Nov 16 2021, 2:11 PM
fwolff requested review of this revision.Nov 16 2021, 2:11 PM
lebedev.ri added inline comments.Nov 16 2021, 2:14 PM
llvm/test/Transforms/DeadStoreElimination/memset-and-strncpy.ll
3

Tests for passes should only test that particular pass, not the whole optimization pipeline.

fwolff updated this revision to Diff 388030.Nov 17 2021, 1:50 PM
fwolff marked an inline comment as done.
xbolva00 added inline comments.Nov 17 2021, 2:34 PM
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1138

Use LLVM_FALLTHROUGH;

1445

Code comment please. Maybe use switch if there are more libcalls which could be added here?

fwolff updated this revision to Diff 388052.Nov 17 2021, 3:40 PM
fwolff marked 2 inline comments as done.
xbolva00 added inline comments.Nov 17 2021, 3:43 PM
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
243

Same here

fwolff updated this revision to Diff 388069.Nov 17 2021, 4:56 PM
fwolff marked an inline comment as done.
fhahn added a comment.Nov 18 2021, 4:28 AM

Is there a way to add the right attributes to strncpy to avoid needing to special case it in DSE?

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
250

Are we missing a return MemoryLocation::getAfter(CB->getArgOperand(0)) here to preserve the existing behavior if *CB is not a libfunc (or not available)?

1442

having this logic here makes the code harder to read. Can this be moved to isReadClobber? Or into BasicAAResult::getModRefInfo directly?

Is there a way to add the right attributes to strncpy to avoid needing to special case it in DSE?

It would be great to somehow connect write_only/read_only with size arg but I dont think this is possible now. GCC’s access attribute is more poweful and it can be used in this way.

Maybe good idea for GSoC?

fhahn added a comment.Nov 18 2021, 8:21 AM

Is there a way to add the right attributes to strncpy to avoid needing to special case it in DSE?

It would be great to somehow connect write_only/read_only with size arg but I dont think this is possible now. GCC’s access attribute is more poweful and it can be used in this way.

Yeah, but I was actually thinking about the special handling for isReadClobber (see inline comments). I don't think the special handling there is needed, because should strncpy already have the required attributes to determine it is not a read clobber.

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1442

Actually this should not be needed, because strncpy already *has* the correct attributes (first arg write only, second arg readonly and function argmemonly), right?

xbolva00 added inline comments.Nov 18 2021, 8:27 AM
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1442

According to BuildLibCalls, yes, right.

fwolff updated this revision to Diff 388268.Nov 18 2021, 10:58 AM
fwolff marked 4 inline comments as done.
fhahn added a comment.Nov 19 2021, 1:59 AM

Thanks for the latest updates!

We already have tests with other library calls in llvm/test/Transforms/DeadStoreElimination/libcalls.ll. It might good to move the tests from llvm/test/Transforms/DeadStoreElimination/memset-and-strncpy.ll to that file and recommit the changes.

Are you planning on also adding support for other *n* variants of library calls?

fhahn added a comment.Nov 19 2021, 2:00 AM

I think the patch also needs a rebase after some recent changes on main.

fwolff updated this revision to Diff 388477.Nov 19 2021, 5:42 AM

@fhahn Fixed and rebased, thanks for the review! (And I'll need someone to commit this for me. You can use name and email as in commit 6259016361345e09f0607ef4e037e00bcbe4bd40 for attribution. Thanks!)

Are you planning on also adding support for other *n* variants of library calls?

Which ones are you thinking of in particular? strncpy is the "easy" one because it always writes exactly n bytes, but I can have a look at some others as well.

fhahn added a comment.Nov 19 2021, 8:19 AM

@fhahn Fixed and rebased, thanks for the review! (And I'll need someone to commit this for me. You can use name and email as in commit 6259016361345e09f0607ef4e037e00bcbe4bd40 for attribution. Thanks!)

Are you planning on also adding support for other *n* variants of library calls?

Which ones are you thinking of in particular? strncpy is the "easy" one because it always writes exactly n bytes, but I can have a look at some others as well.

I committed the tests separately in ffe1741b5ccab07cbf45fe99033e4119688eb5a5. Could you rebase the patch on top of that change?

fwolff updated this revision to Diff 388515.Nov 19 2021, 8:43 AM
fhahn accepted this revision.Nov 19 2021, 9:45 AM

LGTM, thanks! I'll land the change shortly.

This revision is now accepted and ready to land.Nov 19 2021, 9:45 AM
This revision was automatically updated to reflect the committed changes.