Page MenuHomePhabricator

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

Authored by fwolff on Tue, Nov 16, 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.Tue, Nov 16, 2:11 PM
fwolff requested review of this revision.Tue, Nov 16, 2:11 PM
lebedev.ri added inline comments.Tue, Nov 16, 2:14 PM
llvm/test/Transforms/DeadStoreElimination/memset-and-strncpy.ll
2 ↗(On Diff #387760)

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

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

Use LLVM_FALLTHROUGH;

1375

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

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

Same here

fwolff updated this revision to Diff 388069.Wed, Nov 17, 4:56 PM
fwolff marked an inline comment as done.
fhahn added a comment.Thu, Nov 18, 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
223

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)?

1372

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.Thu, Nov 18, 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
1372

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.Thu, Nov 18, 8:27 AM
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1372

According to BuildLibCalls, yes, right.

fwolff updated this revision to Diff 388268.Thu, Nov 18, 10:58 AM
fwolff marked 4 inline comments as done.
fhahn added a comment.Fri, Nov 19, 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.Fri, Nov 19, 2:00 AM

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

fwolff updated this revision to Diff 388477.Fri, Nov 19, 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.Fri, Nov 19, 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.Fri, Nov 19, 8:43 AM
fhahn accepted this revision.Fri, Nov 19, 9:45 AM

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

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