[PATCH] D28084: Value number stores and memory states so we can detect when memorystates are equivalent (IE store of same value to memory).Tests coming.

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

This is very nice. I think I got the latest revision (unless there's an issue with phabricator) and I see many tests in `Transform/NewGVN` failing, some of them hitting the assertion in `updateProcessedCount()`. 
In the meanwhile, some minor comment inline.



================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:793
+  bool Changed = false;
+  // If it's already in the table, see if the value changed
+  if (LookupResult.second) {
----------------
Nit: Comments terminate with a full stop (here and elsewhere).


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1311
+  MemoryAccess *AllSameValue = nullptr;
+  // See if all arguments are the same, ignoring unreachable arguments
+  for (unsigned i = 0, e = MP->getNumOperands(); i < e; ++i) {
----------------
dberlin wrote:
> It would be nice to share this part with performSymbolicPHIEvaluation, but we use a different lookup.
> Thoughts welcome.
> 
I agree it's not great to have the logic in two different places, I'll think about it. Maybe we can add a `FIXME` in the meanwhile?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1327
+  }
+#ifdef DEBUG
+  if (AllSameValue)
----------------
I think the correct spelling is `#ifndef NDEBUG` unless you mean something else (I'm not entirely sure there's a `DEBUG` macro)


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1329
+  if (AllSameValue)
+    dbgs() << "Memory Phi value numbered to " << *AllSameValue << "\n";
+  else
----------------
This could be `DEBUG(dbgs() ...)`


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1445-1452
+      if (MemoryPhi *MP = dyn_cast<MemoryPhi>(V)) {
+        DEBUG(dbgs() << "Processing MemoryPhi " << *MP << "\n");
+        valueNumberMemoryPhi(MP);
+        continue;
+      } else if (Instruction *I = dyn_cast<Instruction>(V)) {
+        valueNumberInstruction(I);
+      } else
----------------
Nit: the style guide suggests to uses braces around the else as well.


https://reviews.llvm.org/D28084