[PATCH] D28934: Write a new SSAUpdater

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

I am obviously a fan of the algorithm and biased :)

- Interesting tweak with the visited set (compared to the paper), guess that saves us RAUWs in many cases at the cost of extra visited set lookups.
- I am surprised to find readVariableBeforeDef() as public API, in my experience outside users of the algorithms do not need it (but maybe you have a good use case). I probably would have named it readVariableAtBlockBegin() or readVariableLiveIn() and the "normal" variant simply readVariable().



================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:59
+    return CDI->second;
+  return readVariableAfterDef(Var, BB);
+}
----------------
Shouldn't this be `readVariableBeforeDef(Var, BB)`?


================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:85
+      // See if we see ourselves, due to a self-cycle
+      if (CurrentDef.lookup({Var, BB}) == PredVal)
+        continue;
----------------
Maybe `PredVal->getParent() == BB && isa<PHINode>(PredVal)` is slightly more efficient.


https://reviews.llvm.org/D28934