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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: include/llvm/Transforms/Scalar/SpeculateAroundPHIs.h:28
+/// materializing the constants along each edge. If the PHI is used by an
+/// instruction where the target can materialize the constant as parte of the
+/// instruction, it is profitable to speculate those instructions around the
----------------
parte -> part


================
Comment at: include/llvm/Transforms/Scalar/SpeculateAroundPHIs.h:30
+/// instruction, it is profitable to speculate those instructions around the
+/// PHI node. This can reduce totaly dynamic instruction count as well as
+/// decrease register pressure.
----------------
I don't think that you need the word totaly (which is also spelled incorrectly).


================
Comment at: include/llvm/Transforms/Scalar/SpeculateAroundPHIs.h:32
+/// decrease register pressure.
+struct SpeculateAroundPHIsPass : PassInfoMixin<SpeculateAroundPHIsPass> {
+  /// \brief Run the pass over the function.
----------------
I think that it would be helpful to show a little pseudo-code example of what this means here.


================
Comment at: lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:55
+  // changing unless there is a store in that same sequence. We should probably
+  // change this to do at least a limited scane of the intervening instructions
+  // and allow handling stores in easily proven safe cases.
----------------
scane -> scan


================
Comment at: lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:200
+    if (auto *UserII = dyn_cast<IntrinsicInst>(UserI))
+      // FIXME: Is the index right for an intrinsic? What about the "function"
+      // operand?
----------------
An outdated comment? The "function" operand is last. Bundle operands are before that. Regular function arguments are first.


================
Comment at: lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:298
+///
+/// This code currently uses a linear to compute order for a greedy approach.
+/// It won't find cases where a set of PHIs must be considered together, but it
----------------
linear to compute -> linear-to-compute (dashes for a compound adjective)


================
Comment at: lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:306
+/// There is an orthogonal extra complexity to all of this: computing the cost
+/// itself can easily become a linear computattion making everything again (at
+/// best) quadratic. Using a postorder over the operand graph makes it
----------------
computattion -> computation


================
Comment at: lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:568
+  // Walk the speculated instruction list and if they have uses, insert a PHI
+  // for them from the speculated versions, and relpcae the uses with the PHI.
+  // Then erase the instructions as they have been fully speculated. The walk
----------------
relpcae -> replace


https://reviews.llvm.org/D37467