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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
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.
----------------
hfinkel wrote:
> I don't think that you need the word totaly (which is also spelled incorrectly).
I totaly don't. And I can totaly spell things just fine. ;]

(Thanks for catching.)


================
Comment at: include/llvm/Transforms/Scalar/SpeculateAroundPHIs.h:32
+/// decrease register pressure.
+struct SpeculateAroundPHIsPass : PassInfoMixin<SpeculateAroundPHIsPass> {
+  /// \brief Run the pass over the function.
----------------
hfinkel wrote:
> I think that it would be helpful to show a little pseudo-code example of what this means here.
I have have gone overboard. Lemme know if you have something else in mind.


================
Comment at: test/Transforms/SpeculateAroundPHIs/X86/basic.ll:141
+; architecture even though they are unrelated to the PHI itself.
+define i32 @test_speculate_free_insts(i1 %flag, i64 %arg) {
+; CHECK-LABEL: define i32 @test_speculate_free_insts(
----------------
davidxl wrote:
> This is simple code hoisting without speculation.
> 
> It is unclear whether this is a win for this case -- it increases code size (and icache pressure). It may or may not reduce register pressure either.
Can you explain how it would increase code size? I'm not really seeing it, but maybe I'm just missing something. All of these `trunc` instructions won't require any materialized code no x86...


As for hoisting vs. speculation -- I can add a call that might never return after the PHI and before the other instructions, and we will still hoist. That becomes, technically, speculative. But see my other email -- happy to use different/better terminology, but so far all of the ideas I've had are less clear.


================
Comment at: test/Transforms/SpeculateAroundPHIs/X86/basic.ll:176
+; profitable because of the total cost.
+define i32 @test_no_spec_multi_uses(i1 %flag, i32 %arg1, i32 %arg2, i32 %arg3) {
+; CHECK-LABEL: define i32 @test_no_spec_multi_uses(
----------------
davidxl wrote:
> What is the cost model here? If there are two uses, it will still be a net win to hoist -- the code size does not change, but the runtime cost is strictly reduced.  If there are more than 2 uses, then there is tradeoff to make between size increase vs runtime cost reduction.
See the code? We actually look at the uses...


https://reviews.llvm.org/D37467