[PATCH] D55467: [WebAssembly] Optimize Irreducible Control Flow

[Alon Zakai via Phabricator via llvm-commits]

kripken marked 12 inline comments as done.
kripken added a comment.

Thanks for the comments @aheejin!

All should be addressed in the patch I'm updating now, or if not then I replied to those comments.



================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:85
     AU.addRequired<MachineDominatorTree>();
     AU.addPreserved<MachineDominatorTree>();
     AU.addRequired<MachineLoopInfo>();
----------------
aheejin wrote:
> I don't think we need dominator tree anymore, so I think we can delete these two lines.
You're right that we don't need it, but it turns out recomputing MLI crashes if I don't keep this code there. I guess the dominator tree is used in MLI, and MLI does not know to recompute it itself? (I don't know anything about LLVM internals...)


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:87
     AU.addRequired<MachineLoopInfo>();
     AU.addPreserved<MachineLoopInfo>();
     MachineFunctionPass::getAnalysisUsage(AU);
----------------
aheejin wrote:
> Do we preserve loop info?
Yes, we preserve it by recomputing it if we invalidate it.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:382
+        MF.RenumberBlocks();
+        getAnalysis<MachineDominatorTree>().runOnMachineFunction(MF);
+        MLI.runOnMachineFunction(MF);
----------------
aheejin wrote:
> Should we recompute these three every time we make a change?
> - This pass does not use liveness info, so I think we can do this at the end of the whole pass if any change was made.
> - I don't think we need to do renumbering? We only use BB numbers at CFGSort / CFGStackify and CFGSort recomputes all numbers itself before doing anything.
> - This pass does not seem to use dominator tree, right? Then deleting `AU.addPreserved<MachineDominatorTree>();` in `getAnalysisUsage()` should be sufficient. I don't think we even need to require it.
We need to recompute the loop info each time since we use it, which I think means we need to recompute the others (like dominator trees, see comment above).

In practice, often there is no irreducible control flow, so no recomputing is done. Or it can be resolved in one iteration, in which case we recompute once as needed. It's rare to need more work than that.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:383
+        getAnalysis<MachineDominatorTree>().runOnMachineFunction(MF);
+        MLI.runOnMachineFunction(MF);
+        return Changed = true;
----------------
aheejin wrote:
> Should we recompute loop info for the whole function every time we make any change? `MachineLoopInfo` class seems to have [[ https://github.com/llvm-mirror/llvm/blob/2ca3d46c8a27e6de8a99de6bdc08e0884e8e6a85/include/llvm/CodeGen/MachineLoopInfo.h#L142-L169 | various modifier functions ]]. Can we use these on-the-fly during the pass?
I'd prefer not to as it seems more complex than worthwhile, given the comment above on how much work is done in the common cases here. But if you feel strongly I can do that.


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

https://reviews.llvm.org/D55467