This is an archive of the discontinued LLVM Phabricator instance.

[DSE] Remove alloc function check in canSkipDef()
ClosedPublic

Authored by nikic on Jan 11 2022, 3:36 AM.

Details

Summary

canSkipDef() currently skips inaccessiblememonly calls, but not if they are allocation functions. This check was added in D103009, but I don't understand why. There are no test failures if it is removed, and I don't really see what relation it has to the implemented transform (as canSkipDef() is not used on the storeIsNoop() path, as far as I can tell).

Either this is dead code, or we're missing a test.

Diff Detail

Event Timeline

nikic created this revision.Jan 11 2022, 3:36 AM
nikic requested review of this revision.Jan 11 2022, 3:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2022, 3:36 AM

FWIW, I'm also quite confused by this code. My best guess was that maybe this was handling the case where we introduced a new memory region (e.g. the return noalias) as opposed to merely writing to an existing region, but that's somewhat a guess. I don't see that distinction made in the DSE code, and I'm not sure how memory-ssa models that.

Hi, it appears that https://reviews.llvm.org/D103009 emerged from https://reviews.llvm.org/D101440 and canSkipDef change was included by mistake.
It should be safe to get rid of it. Also together with isAllocLikeFn we can remove TLI parameter I think.

fhahn accepted this revision.Jan 16 2022, 8:33 AM

LGTM, thanks!

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

as @yurai007 suggested, it looks like the TLI arg can be dropped

This revision is now accepted and ready to land.Jan 16 2022, 8:33 AM
This revision was automatically updated to reflect the committed changes.