[PATCH] D28934: Write a new SSAUpdater

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: include/llvm/Transforms/Utils/SSAUpdaterNew.h:1-2
+//===-- SSAUpdaterNew.h - Unstructured SSA Update Tool -------------*- C++
+//-*-===//
+//
----------------
Unformatted, I think.


================
Comment at: include/llvm/Transforms/Utils/SSAUpdaterNew.h:37
+
+/// \brief Helper class for SSA formation on a set of values defined in
+/// multiple blocks.///
----------------
davide wrote:
> extra `///`
The whole comment needs formatting. The explanation is very good, IMHO.


================
Comment at: include/llvm/Transforms/Utils/SSAUpdaterNew.h:37-38
+
+/// \brief Helper class for SSA formation on a set of values defined in
+/// multiple blocks.///
+/// This is used when code duplication or another unstructured
----------------
extra `///`


================
Comment at: include/llvm/Transforms/Utils/SSAUpdaterNew.h:42-43
+/// set of values.
+/// First, choose an unsigned int to represent your "variable".  As long as you
+/// are consistent, it doesn not matter which you choose.
+/// Set the name and type of the "variable" using setName and setType.
----------------
s/doesn/does/


================
Comment at: include/llvm/Transforms/Utils/SSAUpdaterNew.h:88-92
+  SmallVectorImpl<PHINode *> *InsertedPHIs;
+  DenseMap<std::pair<unsigned, BasicBlock *>, Value *> CurrentDef;
+  DenseSet<std::pair<unsigned, BasicBlock *>> VisitedBlocks;
+  DenseMap<unsigned, Type *> CurrentType;
+  DenseMap<unsigned, StringRef> CurrentName;
----------------
Comments on these member variables, maybe? (as we do in NewGVN).


================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:1-2
+//===- SSAUpdaterNew.cpp - Unstructured SSA Update Tool
+//----------------------===//
+//
----------------
Unformatted.


================
Comment at: lib/Transforms/Utils/SSAUpdaterNew.cpp:48-56
+void SSAUpdaterNew::setType(unsigned Var, Type *Ty) { CurrentType[Var] = Ty; }
+void SSAUpdaterNew::setName(unsigned Var, StringRef Name) {
+  CurrentName[Var] = Name;
+}
+
+// Store a def of Variable Var, in BB, with Value V
+void SSAUpdaterNew::writeVariable(unsigned Var, BasicBlock *BB, Value *V) {
----------------
These functions appear to be trivial, move to the header, maybe?


https://reviews.llvm.org/D28934