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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Some comments on the software engineering side, I didn't really look into the dependency breaking algorithms yet.



================
Comment at: include/llvm/CodeGen/BreakFalseDeps.h:36
+
+class BreakFalseDeps : public MachineFunctionPass, LoopTraversal {
+private:
----------------
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.


================
Comment at: include/llvm/CodeGen/BreakFalseDeps.h:82
+private:
+  // Dependency breaking.
+  bool pickBestRegisterForUndef(MachineInstr *MI, unsigned OpIdx,
----------------
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
/// \{
...
/// \}
```



================
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
----------------
- 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.


================
Comment at: include/llvm/CodeGen/LoopTraversal.h:51-67
+  struct MBBInfo {
+    // Whether we have gotten to this block in primary processing yet.
+    bool PrimaryCompleted = false;
+
+    // The number of predecessors for which primary processing has completed
+    unsigned IncomingProcessed = 0;
+
----------------
This looks to me like these datastructures could be `private`.


================
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);
----------------
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.


================
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) {
----------------
It is usually more convenient for users of the API if the doxygen comments are in the header;  similar for following methods.


================
Comment at: lib/CodeGen/LoopTraversal.cpp:21-23
+  enterBasicBlock(MBB);
+  processBasicBlock(MBB, PrimaryPass);
+  leaveBasicBlock(MBB);
----------------
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.


================
Comment at: lib/CodeGen/LoopTraversal.cpp:36
+  // Initialize the MMBInfos
+  for (auto &MBB : mf) {
+    MBBInfo InitialInfo;
----------------
Using `MachineBasicBlock &` instead of `auto &` is friendlier to people reading the code.


================
Comment at: lib/CodeGen/LoopTraversal.cpp:45-48
+  for (ReversePostOrderTraversal<MachineBasicBlock *>::rpo_iterator
+           MBBI = RPOT.begin(),
+           MBBE = RPOT.end();
+       MBBI != MBBE; ++MBBI) {
----------------
You could simplify this to:
```
for (MachineBasicBlock *MBB : RPOT) {
   // ...
}
```

Same with the loop after this one.


================
Comment at: lib/CodeGen/LoopTraversal.cpp:63-71
+          if (Primary) {
+            MBBInfos[Succ].IncomingProcessed++;
+          }
+          if (Done) {
+            MBBInfos[Succ].IncomingCompleted++;
+          }
+          if (isBlockDone(Succ)) {
----------------
Don't use `{}` for 1 line ifs.


================
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.
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D40333