[PATCH] D40333: Separate LoopTraversal and BreakFalseDeps out of ExecutionDomainFix into their own files

[Marina Yatsina via Phabricator via llvm-commits]

myatsina marked an inline comment as done.
myatsina added a comment.

Hi,

Thanks for the comments!
My changes are mostly refactoring of the old ExecutionDepsDix pass in a way that is (almost*) NFC.
This pass combined the logic of both breaking false dependencies and execution domain adjustments in one single pass that traversed the basic blocks in a very special way (optimizing traversal of loops).
I think the first review (https://reviews.llvm.org/D40330) in this bunch would make everything clearer for you.

In order to fix the bugzilla 33869 I had to make a small adjustment for the breaking false dependencies part, but because all this logic was inter-twined I had to first separate it.
The new logic I've added is pretty much summarized here https://reviews.llvm.org/D40334.

*almost NFC - The logic I've changed in all this refactoring compared to the original ExecutionDepsFix is that I've enabled the "break false dependecy" part of the logic to run on all register classes, and not only the register class that ExecutionDepsFix received in its c'tor.

I will definitely adopt some of you refactoring suggestions and upload a patch soon.

Thanks,
Marina



================
Comment at: include/llvm/CodeGen/BreakFalseDeps.h:36
+
+class BreakFalseDeps : public MachineFunctionPass, LoopTraversal {
+private:
----------------
MatzeB wrote:
> This is probably a bad use of inheritance. Saying "BreakFalseDeps is a LoopTraversal" feels odd. I believe this could be rewritten so that LoopTraversal is subclassed separately inside BreakFalseDeps.cpp without making it a supertype of BreakFalseDeps itself.

Both BreakFalseDeps and ExecutionDomainFix traverse the basic blocks in a special way.
The LoopTraversal provides the traversal algorithm and methods that allow inheriting passes to handle instructions, handle BBs, etc.
Do you have a better suggestion on how to do it without duplicating the traversal logic in both passes?


================
Comment at: include/llvm/CodeGen/BreakFalseDeps.h:82
+private:
+  // Dependency breaking.
+  bool pickBestRegisterForUndef(MachineInstr *MI, unsigned OpIdx,
----------------
MatzeB wrote:
> It's usually easiest to just document the functions with `///` doxygen comments instead of forming groups of methods.
> 
> If you really want to form groups, the correct doxygen syntax would be:
> ```
> /// \name Dependency Breaking
> /// \{
> ...
> /// \}
> ```
> 
Some of these methods are documented in the cpp file.
This is how they were originally documented when they were part of the ExecutionDepsFix pass.
Do you think we take this refactoring a a chance to move all method documentation to the header file?


================
Comment at: include/llvm/CodeGen/LoopTraversal.h:12-39
+/// This class implements the basic blocks traversal order used by passes like
+/// BreakFalseDeps and ExecutionDomainFix.
+///
+/// We want to visit every instruction in every basic block in order to update
+/// it's execution domain or break any false dependencies. However, for the
+/// dependency breaking, we need to know clearances from all predecessors
+/// (including any backedges). One way to do so would be to do two complete
----------------
MatzeB wrote:
> - The comment (except for the first `\file` sentence) would better fit in front of `class LoopTraversal`.
> 
> - It's good to see an example trying to explain the goals of the algorithm!
> 
> - You can simplify the explanation by removing descriptions of how things are not done (i.e. the "it's not using the two-pass or naive worklist approaches" parts). Instead you should describe what actually is done: A reverse postorder traversal with an intertwined worklist algorithm for backedges.
I think that mentioning the naive approach and what we are trying to optimize is actually making this traversal algorithm (and the motivation for it) much clearer.
I can refactor the comment so that we have "The algorithm does ..." and at the end add a few sentences like "A naive way of implementing this would have been ..., but the downside of this is ..., this is why the optimized way was chosen", or something of that sort.
Do you think this way will be better?


================
Comment at: include/llvm/CodeGen/LoopTraversal.h:73-81
+  // Overridden by the derived classes.
+  virtual void enterBasicBlock(MachineBasicBlock *) = 0;
+  virtual void leaveBasicBlock(MachineBasicBlock *) = 0;
+  virtual void processBasicBlock(MachineBasicBlock *MBB, bool PrimaryPass) = 0;
+
+  // Traversal operations
+  void runOnBasicBlock(MachineBasicBlock *MBB, bool PrimaryPass);
----------------
MatzeB wrote:
> This API seems too complicated to me. Given that this is just a loop traversal API I would expect that `traverse()` and `runOnBasicBlock()` is enough.
> 
> See also my comment inside `runOnBasicBlock()`; `isBlockDone()` can be private so it is not part of the API I believe.
I was trying to avoid duplicating runOnBasicBlock() logic for the inheriting classes.
It is not a lot of duplication, it basically just activates the other 3 methodes (enterBB, processBB, leaveBB) and that's it, so we can simplify the API as you suggested and have both inheriting class implement runOnBB the same way.



================
Comment at: lib/CodeGen/LoopTraversal.cpp:18-19
+
+/// Includes the general logic of handeling a basic block, activating sub phases
+/// that are implemented by the derived classes.
+void LoopTraversal::runOnBasicBlock(MachineBasicBlock *MBB, bool PrimaryPass) {
----------------
MatzeB wrote:
> It is usually more convenient for users of the API if the doxygen comments are in the header;  similar for following methods.
Sure, I'll move them.
I've was trying to keep it close to what was in the original ExecutionDepsFix header and cpp..


================
Comment at: lib/CodeGen/LoopTraversal.cpp:21-23
+  enterBasicBlock(MBB);
+  processBasicBlock(MBB, PrimaryPass);
+  leaveBasicBlock(MBB);
----------------
MatzeB wrote:
> As far as I can see all three of `enterBasicBlock()`, `processBasicBlock()`, `leaveBasicBlock()` are always called together in this order. So it could just as well be a single function. In fact I believe just adding `virtual` to `runOnBasicBlock()` and removing the three other methods would suffice.
I can definitely do that, just keep in mind that this way you will see some duplication because both inheriting classes will implement it the same way (calling their own private enterBB, processBB and leaveBB).


================
Comment at: lib/CodeGen/LoopTraversal.cpp:78-80
+  // We need to go through again and finalize any blocks that are not done yet.
+  // This is possible if blocks have dead predecessors, so we didn't visit them
+  // above.
----------------
MatzeB wrote:
> Can you explain how this happens? Both loops appear to be using a RPO traversal, how could the first one miss some that this 2nd pass catches?
Keno Fischer (@loladiro ) is the author of this code in ExecutionDepsFix.
Keno, can you explain?



Repository:
  rL LLVM

https://reviews.llvm.org/D40333