[PATCH] D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable.

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.

LGTM

> I've also only implemented this in the new pass manager. If folks are

very interested, I can try to add it to the old PM as well, but I didn't
really see much point (my use case is already switched over to the new
PM).

Isn't the legacy PM still the default we use for most frontends including clang and the new PM is only used for (Thin)LTO so far?



================
Comment at: llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:53-58
+  auto *PhiBB = PN.getParent();
+  SmallPtrSet<Instruction *, 4> Visited;
+  SmallVector<std::pair<Instruction *, User::value_op_iterator>, 16> DFSStack;
+
+  // Walk each user of the PHI node.
+  for (auto &U : PN.uses()) {
----------------
Too much `auto` for my taste. I think writing the types is friendlier to code readers (unless you have something like a `cast<XXX>` on the RHS which makes the type obvious.


================
Comment at: llvm/test/Transforms/SpeculateAroundPHIs/X86/lit.local.cfg:1-2
+if not 'X86' in config.root.targets:
+    config.unsupported = True
----------------
As this is only a single test, you could use `REQUIRES: x86-registered-target` in the test itself instead of creating an x86 specific subdirectory.


https://reviews.llvm.org/D37467