[PATCH] D28934: Write a new SSAUpdater

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

I read the paper and reviewed the marker algorithm. Some comments inline.
Side note, for didactical purposes: I wasn't familiar with this algorithm before, but I think it's very nice, probably easier to understand than Morgan's book chapter/algs on SSA construction.



================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:40-44
+using namespace llvm;
+
+#define DEBUG_TYPE "SSAUpdaterNew"
+
+SSAUpdaterNew::SSAUpdaterNew(SmallVectorImpl<PHINode *> *NewPHI)
----------------
I'll be happy if we can add stats about the number of PHIs trivialized, inserted etc.. (so that we can also compare the "basic" marker algorithm and the SCC approach)


================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:78-85
+// This is the marker algorithm from "Simple and Efficient Construction of
+// Static Single Assignment Form"
+// The simple, non-marker algorithm places phi nodes at any join
+// Here, we place markers, and only place phi nodes if they end up necessary.
+// They are only necessary if they break a cycle (IE we recursively visit
+// ourselves again), or we discover, while getting the value of the operands,
+// that there are two or more definitions needing to be merged.
----------------
You can point out here (or elsewhere) that the updater can be somehow trivially extended to handle incomplete CFGs.


================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:93-96
+  } else if (VisitedBlocks.count({Var, BB})) {
+    // We hit our node again, meaning we had a cycle, we must insert a phi
+    // node to break it.
+    IRBuilder<> B(BB, BB->begin());
----------------
As you're describing the single predecessor case above, I would add a comment here saying something like: `Multiple predecessors case, collect all the definitions and construct a phi, if necessary`


================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:99
+  } else {
+    // Mark us visited so we can detect a cycle
+    VisitedBlocks.insert({Var, BB});
----------------
dot at the end of comment, here and everywhere else.


================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:130
+    if (InsertedPHIs)
+      if (PHINode *PN = dyn_cast<PHINode>(Result))
+        InsertedPHIs->push_back(PN);
----------------
`auto *`, here and everywhere else you use `dyn_cast<>`, `dyn_cast_or_null<>`, `cast<>` etc..


================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:148-149
+    if (PHINode *UsePhi = dyn_cast<PHINode>(&*U)) {
+      auto OperRange = UsePhi->operands();
+      tryRemoveTrivialPhi(Var, UsePhi, OperRange);
+    }
----------------
Just fold `OperRange` inside `tryRemoveTrivialPhi`, maybe.


================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:174-176
+  // Never found a non-self reference, the phi is undef
+  if (Same == nullptr)
+    return UndefValue::get(CurrentType.lookup(Var));
----------------
It would be good if we can add a test showing that we trivialize self-referencing phi, if possible.


================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:175-176
+  // Never found a non-self reference, the phi is undef
+  if (Same == nullptr)
+    return UndefValue::get(CurrentType.lookup(Var));
+  if (Phi) {
----------------
Also, I understand that we don't need to call `replaceAllUsesWith` as there are no non-self uses, but shouldn't we call `eraseFromParent` here?


================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:188-192
+// Wish we could use SCCIterator, but graph traits makes it *very* hard to
+// create induced subgraphs because it
+// 1. only works on pointers, so i can't just create an intermediate struct
+// 2. the functions are static, so i can't just override them in a subclass of
+// graphtraits, or otherwise store state in the struct.
----------------
This is not the first time that I find SCCIterator being not-very-general.


https://reviews.llvm.org/D28934