[PATCH] D24805: [GVNSink] Initial GVNSink prototype

[Matthias Braun via llvm-commits]

MatzeB added a comment.

Just some coding style nitpicking.

- Still a bunch of FIXME comments around. I often feel that many of them could just as well have been removed if they didn't turn out out to be important enough to be addressed for the commit.


================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:10-32
@@ +9,25 @@
+//
+// This pass attempts to sink instructions into successors, reducing static
+// instruction count and enabling if-conversion.
+//
+// We use a variant of global value numbering to decide what can be sunk.
+// Consider:
+//
+// [ %a1 = add i32 %b, 1  ]   [ %c1 = add i32 %d, 1  ]
+// [ %a2 = xor i32 %a1, 1 ]   [ %c2 = xor i32 %c1, 1 ]
+//                  \           /
+//            [ %e = phi i32 %a2, %c2 ]
+//            [ add i32 %e, 4         ]
+//
+//
+// GVN would number %a1 and %c1 differently because they compute different
+// results - the VN of an instruction is a function of its opcode and the
+// transitive closure of its operands. This is the key property for hoisting
+// and CSE.
+//
+// What we want when sinking however is for a numbering that is a function of
+// the *uses* of an instruction, which allows us to answer the question "if I
+// replace %a1 with %c1, will it contribute in an equivalent way to all
+// successive instructions?". The PostValueTable class in GVN provides this
+// mapping.
+//
----------------
This could be

```
/// \file
/// This pass attempts...
```
to make the comment available to doxygen.

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:68-79
@@ +67,14 @@
+
+// LockstepReverseIterator - Iterates through instructions in a set of blocks in
+// reverse order from the first non-terminator.
+// For example (assume all blocks have size n):
+//   LockstepReverseIterator I([B1, B2, B3]);
+//   *I-- = [B1[n], B2[n], B3[n]];
+//   *I-- = [B1[n-1], B2[n-1], B3[n-1]];
+//   *I-- = [B1[n-2], B2[n-2], B3[n-2]];
+//   ...
+//
+// It continues until all blocks have been exhausted. Use \c getActiveBlocks() to
+// determine which blocks are still going and the order they appear in the
+// list returned by operator*.
+class LockstepReverseIterator {
----------------
`///` for doxygen, do not repeat the type name in the comment.

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:93
@@ +92,3 @@
+    ActiveBlocks.clear();
+    for (auto *BB : Blocks)
+      ActiveBlocks.insert(BB);
----------------
A matter of taste, but I think `BasicBlock *BB` instead of `auto *BB` is only slightly longer and has a higher documentation value. Similar in many following for loops.

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:113
@@ +112,3 @@
+  void restrictToBlocks(SmallPtrSetImpl<BasicBlock*> &Blocks) {
+    auto II = Insts.begin();
+    for (; II != Insts.end();) {
----------------
I'd move that into the for loop to make the cope obvious.

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:129
@@ +128,3 @@
+    SmallVector<Instruction*,4> NewInsts;
+    for (auto *&Inst : Insts) {
+      if (Inst == &Inst->getParent()->front())
----------------
Using a reference seems unnecessary (I don't see any modification).

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:171
@@ +170,3 @@
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
+                              SinkingInstructionCandidate &C) {
+  OS << "<Candidate Cost=" << C.Cost << " #Blocks=" << C.NumBlocks
----------------
could be `const`

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:187
@@ +186,3 @@
+  ModelledPHI() {}
+  ModelledPHI(PHINode *PN) {
+    for (unsigned I = 0, E = PN->getNumIncomingValues(); I != E; ++I)
----------------
could be `const PHINode &`

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:198
@@ +197,3 @@
+
+  // Create a PHI from an array of incoming values and incoming blocks.
+  template <typename VArray, typename BArray>
----------------
doxygen `///`. More cases of this follow.

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:250
@@ +249,3 @@
+};
+typedef std::unordered_set<ModelledPHI, /*Hash=*/ModelledPHI> ModelledPHISet;
+
----------------
Is there anything missing in llvms `DenseSet` or any other reason for `std::unordered_set`?


================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:523
@@ +522,3 @@
+    for (auto *V : NewPHI.getValues())
+	if (PHIContents.count(V))
+          // V exists in this PHI, but the whole PHI is different to NewPHI
----------------
Indentation

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:610
@@ +609,3 @@
+    ModelledPHISet NeededPHIs;
+    std::set<Value*> PHIContents;
+    analyzeInitialPHIs(BBEnd, NeededPHIs, PHIContents);
----------------
Why not `SmallPtrSet`?

================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:624-625
@@ +623,4 @@
+
+    std::stable_sort(Candidates.begin(), Candidates.end());
+    std::reverse(Candidates.begin(), Candidates.end());
+    DEBUG(
----------------
A custom reversed comparison operator should save the reverse() operation.


https://reviews.llvm.org/D24805