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

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

Just some initial comments around the test cases.

It seems that all test cases are testing non speculative code hoisting -- there are no speculative hoisting tests.



================
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(
----------------
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.


================
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(
----------------
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.


https://reviews.llvm.org/D37467