Page MenuHomePhabricator

[MemorySSAUpdater] Mark non-trivial Phi for not to optimize
ClosedPublic

Authored by zzheng on Mar 8 2018, 9:54 AM.

Details

Summary

Fix PR36484, as suggested:

<quote>
during moves, mark the direct users of the erased things that were phis as "not to be optimized"
<quote>

Diff Detail

Repository
rL LLVM

Event Timeline

zzheng created this revision.Mar 8 2018, 9:54 AM

Please commit the spelling corrections in lib/Analysis/MemorySSA.cpp separately.

This looks right, but I'm not that familiar with the algorithm; hopefully Daniel will chime in.

include/llvm/Analysis/MemorySSAUpdater.h
65

Can you use AssertingVH<MemoryPHI> here? I don't think you can get any dangling pointers with your algorithm, but it's nice to add some extra checks.

dberlin accepted this revision.Apr 4 2018, 12:05 PM

This looks right to me (IE if it's wrong, i'm wrong about what will work, it looks implemented correctly)

This revision is now accepted and ready to land.Apr 4 2018, 12:05 PM

Thanks for the patch!

Just one nit (inline) and a general question: is this new set meant to persist across multiple updates, should the erase in fixupDefs be taking care of everything added in a given query, or should we be clearing it at some point?

(If the answer is "the middle one," would it be easy to assert that the def is empty somewhere?)

Please commit the spelling corrections in lib/Analysis/MemorySSA.cpp separately

+1.

lib/Analysis/MemorySSAUpdater.cpp
329

Nit: Can the count check be removed entirely?

zzheng updated this revision to Diff 141098.Apr 4 2018, 7:25 PM
zzheng added a reviewer: zinob.

Addressed comments by Eli and George.

zzheng marked 2 inline comments as done.EditedApr 4 2018, 7:28 PM

Just one nit (inline) and a general question: is this new set meant to persist across multiple updates, should the erase in fixupDefs be taking care of everything added in a given query, or should we be clearing it at some point?

(If the answer is "the middle one," would it be easy to assert that the def is empty somewhere?)

At least two GVN-hoist test cases need NonOptPhis be cleared. The patch is scoped at per call of MemorySSAUpdater::moveTo(), so clean-up is placed at the end of this function.

zzheng added a comment.Apr 4 2018, 7:29 PM

I'll run a bunch of internal test to validate the patch. I'll commit it after testing if no one objects.

zzheng closed this revision.Apr 9 2018, 2:01 PM

committed as r329621, spelling errors fixed in r329230