[PATCH] D132450: StructurizeCFG: Set Undef for non-predecessors in setPhiValues()

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:687-689
+              // If this is not flow node and not the incoming block before
+              // structurization, then this block does not contribute any value
+              // to the phi.
----------------
ruiling wrote:
> ruiling wrote:
> > sameerds wrote:
> > > This is the core idea. But it's not obvious how the code implements this. Would be good to have an explanation of the Stack and the Incomings set. For example, why do we need to erase Current from Incomings? Somewhere in the traversal, the Stack seems to ensure that undefs are added to phis at intermediate Flow blocks ... needs explanation of why it always works.
> > Sorry I don't quite get your point. The reason we need to erase Current from I
> The reason I was erasing Current from Incomings is to try to terminate the traversal loop after finishing all the incoming predecessors. But now I think this maybe not quite necessary. The Stack is just used as traversal stack, no other meaning.
What I mean is that the code needs comments explaining how it achieves the goal. There is a stack, so some form of DFS is happening. But it will be good to have an explanation (in the source file itself, and not this review) of why the implementation is correct. This is for future reference. It is not enough to expect that anyone reading the code will "eventually" figure out how it is all working.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132450/new/

https://reviews.llvm.org/D132450