[PATCH] D26224: NewGVN

Sean Silva via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 12 22:14:42 PST 2016


silvas added a comment.

A couple more small comments as take another pass looking at the patch.



================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:842
+
+/// performSymbolicEvaluation - Substitute and symbolize the value
+/// before value numbering.
----------------
"Don’t duplicate function or class name at the beginning of the comment."

http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1455
+
+    if (DFSIn < other.DFSIn)
+      return true;
----------------
This is just implementing a lexicographical comparison, right?

If so, I would do:

```
return std::tie(DFSIn, DFSOut, ...) < std::tie(other.DFSIn, other.DFSOut, ...);
```


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1464
+        else if (LocalNum == other.LocalNum) {
+          if (Val < other.Val)
+            return true;
----------------
This seems super sketchy, comparing < on pointers. What's up with that? Won't that make the result nondeterministic?


https://reviews.llvm.org/D26224





More information about the llvm-commits mailing list