[PATCH] D153014: Deduplication of cyclic PHI nodes

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

I like the idea here, but the implementation approach is somewhat iffy. This is only going to work in the "naive" implementation -- I don't think the hash-based implementation can be extended to support this. So it won't work if there are many phis. The implementation approach is also quite inefficient: We're performing pairwise, recursive comparisons of all phis.

It should be possible to do this more generally and efficiently (something based on equivalence classes with optimistic assumption), but that would require processing all phis in the function together, not just those in a single block.

A substantial limitation here is that this only works on pure phi graphs, and for example won't be able to deduplicate typical induction variables where there is something like an `add %phi, 1` in between. But at that point we're getting into phi-aware GVN, which is ... tricky.

I'm generally open to having this simple approach, as long as it can be made essentially free in terms of compile-time impact.



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1263-1265
+  bool Inserted = MatchingPhis.insert(std::make_pair(P1, P2)).second;
+  assert(Inserted && "Already visited?");
+  (void)Inserted;
----------------
Can do something like this can drop the `MatchingPhis.count()` calls below.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1277
+    if (auto *I1Phi = dyn_cast<PHINode>(I1))
+      if (auto *I2Phi = dyn_cast_or_null<PHINode>(I2))
+        if (I1Phi->getParent() == I2Phi->getParent())
----------------
Just dyn_cast.


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


================
Comment at: llvm/unittests/Transforms/Utils/LocalTest.cpp:186
+  }
+}
+
----------------
Use a lit test please, in some pass that uses these utilities.


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

https://reviews.llvm.org/D153014