[PATCH] D29316: Add predicateinfo intrinsic, analysis pass, and basic NewGVN support

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek added a comment.

minor fixes



================
Comment at: include/llvm/Transforms/Utils/PredicateInfo.h:156-157
+class PredicateInfo {
+public:
+private:
+  // Used to store information about each value we might rename.
----------------
redundant. 


================
Comment at: include/llvm/Transforms/Utils/PredicateInfo.h:244
+  struct Result {
+    Result(std::unique_ptr<PredicateInfo> &&PredInfo)
+        : PredInfo(std::move(PredInfo)) {}
----------------
take unique_ptr by value. This ensures that the pointer value sinks (and it is also less characters to read, and it is easier for the optimizer to optimize it) 


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1103
+                                                       const BasicBlock *B) {
+  CmpInst *CI = dyn_cast<CmpInst>(I);
+  // See if our operands are equal to those of a previous predicate, and if so,
----------------
auto *


================
Comment at: lib/Transforms/Utils/PredicateInfo.cpp:86-87
+  mutable DenseMap<const BasicBlock *, OrderedBasicBlock *> OBBMap;
+  ValueDFS_Compare(DenseMap<const BasicBlock *, OrderedBasicBlock *> &OBBMap)
+      : OBBMap(OBBMap) {}
+  bool operator()(const ValueDFS &A, const ValueDFS &B) const {
----------------
Is it ok here to copy the map?


================
Comment at: lib/Transforms/Utils/PredicateInfo.cpp:134-135
+    // guaranteed they are in the same block if they are instructions.
+    Argument *ArgA = dyn_cast_or_null<Argument>(ADef);
+    Argument *ArgB = dyn_cast_or_null<Argument>(BDef);
+
----------------
auto


================
Comment at: lib/Transforms/Utils/PredicateInfo.cpp:260
+  for (auto Comparison : ComparisonsToProcess) {
+    if (CmpInst *Cmp = dyn_cast<CmpInst>(Comparison)) {
+      collectCmpOps(Cmp, CmpOperands);
----------------
auto


================
Comment at: lib/Transforms/Utils/PredicateInfo.cpp:556
+}
+PredicateInfo::~PredicateInfo() {
+  for (auto KV : OBBMap)
----------------
extra line betwwen


================
Comment at: lib/Transforms/Utils/PredicateInfo.cpp:557-558
+PredicateInfo::~PredicateInfo() {
+  for (auto KV : OBBMap)
+    delete KV.second;
+}
----------------
Why just not store unique_ptrs inside the map?


https://reviews.llvm.org/D29316