[PATCH] D28522: Codegen: Make chains from lattice-shaped CFGs

[Justin Lebar via Phabricator via llvm-commits]

jlebar added a comment.

Kyle asked me to look over this from an outsider's perspective and check for understandability.  But after starting on it, I think this would be easier after David's comments have been addressed.  I just have a few comments for now.



================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:821
+/// and for 3 or greater they are very uncommon and complex to compute
+/// optimally.
+bool MachineBlockPlacement::isLattice(
----------------
One thing that's confusing to me is: What exactly is "in" the lattice?

We say that BB is part of the lattice if its successors form a lattice.  So it sounds like a recursive definition.  But then the criteria for whether BB's successors "form a lattice" is different from the criteria for whether BB is itself in a lattice.

The other thing I have no intuition for here is, why do we use the mathematical word "lattice" for this shape?  I'm sure there's a good reason, and understanding that might help in general.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1000
+      if (Successors.size() > 1
+          && hasSameSuccessors(*Pred, Successors))
+        // This will result in a lattice after tail duplication, so we don't
----------------
Can we run clang-format over this patch?  git-clang-format, included in the clang sources, can run it just over your changes, so you don't have to reformat the whole file.

For instance, this line appears to fit in 80 chars, so doesn't need to be wrapped.  Also we usually put "&&" at the end of the line, not the beginning.  And there are some lines that appear to be longer than 80 chars.

If it helps, I have a script that runs git-clang-format on every arc diff so I don't have to remember to do it myself.  https://github.com/jlebar/conf/blob/master/bin/arc


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1308
+        BB, Successors, AdjustedSumProb, Chain, BlockFilter);
+  }
+
----------------
Nit, we usually omit these braces, even though the if-body is multiline.


https://reviews.llvm.org/D28522