[PATCH] D153014: Deduplication of cyclic PHI nodes

[Marek Sedláček via Phabricator via llvm-commits]

mark-sed added a comment.

@nikic

> The implementation approach is also quite inefficient: We're performing pairwise, recursive comparisons of all phis.

Recursion is of course not ideal, but the comparison has an upper limit. Would you suggest trying to make this into an iterative approach?



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1382
+      assert(Result2 == Result && "Must be symmetric");
+#endif
       return Result;
----------------
nikic wrote:
> This won't work, and violates the DenseMap contract, because it will report two phis as equal that have a different hash.
That is a great point. I will look into a different approach.


================
Comment at: llvm/unittests/Transforms/Utils/LocalTest.cpp:186
+  }
+}
+
----------------
nikic wrote:
> Use a lit test please, in some pass that uses these utilities.
I wanted to use a lit test, but this function is called only in the -simplifycfg pass, but that pass does way too many optimizations. Using a unit test I can easily test that exactly this feature works and also it allows me to create a smaller test, where I don't have to worry about simplifycfg removing some parts.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153014/new/

https://reviews.llvm.org/D153014