Skip to content

Commit

Permalink
Remove overlapping-write simplification from RP builder.
Browse files Browse the repository at this point in the history 
This is essentially a revert of http://review.skia.org/684676

This code still finds a handful of wins in some of our contrived
test code, but http://review.skia.org/686636 is a more effective
optimization overall, and it doesn't feel worth it to keep both.

Change-Id: I33411079f560af20445aedbaf8cbf208f5156cc5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/686537
Reviewed-by: Arman Uguray <[email protected]>
Commit-Queue: John Stiles <[email protected]>
  • Loading branch information
@johnstiles-google
johnstiles-google authored and SkCQ committed Apr 28, 2023
1 parent d01dd1f commit 90e1ec7
Show file tree
Hide file tree
Showing 8 changed files with 23 additions and 39 deletions.
33 changes: 1 addition & 32 deletions src/sksl/codegen/SkSLRasterPipelineBuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -822,33 +822,10 @@ static bool slot_ranges_overlap(SlotRange x, SlotRange y) {
y.index < x.index + x.count;
}

void Builder::simplifyOverwrittenRange(SlotRange dst) {
while (!fInstructions.empty()) {
// Check if the last instruction is writing a constant. (Any write is potentially a
// candidate for this optimization, but in practice, 99% of the benefit comes from
// eliminating zero-initialization of newly-declared variables that are assigned on the next
// line.)
Instruction& lastInstruction = fInstructions.back();
if (lastInstruction.fOp != BuilderOp::copy_constant) {
return;
}

SlotRange last = {lastInstruction.fSlotA, lastInstruction.fImmA};
if (dst.index <= last.index && dst.index + dst.count >= last.index + last.count) {
// The overwrite range totally encompasses the last instruction's write range;
// the last instruction is dead code and can be eliminated.
fInstructions.pop_back();
} else {
// We can't simplify further.
return;
}
}
}

void Builder::copy_constant(Slot slot, int constantValue) {
// If the last instruction copied the same constant, just extend it.
if (!fInstructions.empty()) {
Instruction lastInstr = fInstructions.back();
Instruction& lastInstr = fInstructions.back();

// If the last op is copy-constant...
if (lastInstr.fOp == BuilderOp::copy_constant &&
Expand All @@ -858,18 +835,10 @@ void Builder::copy_constant(Slot slot, int constantValue) {
lastInstr.fSlotA + lastInstr.fImmA == slot) {
// ... then we can extend the copy!
lastInstr.fImmA += 1;

// If the previous instruction was writing to this range, it's dead code.
fInstructions.pop_back();
this->simplifyOverwrittenRange({lastInstr.fSlotA, lastInstr.fImmA});

// Put back the copy-constant instruction with the newly increased range.
fInstructions.push_back(lastInstr);
return;
}
}

this->simplifyOverwrittenRange({slot, 1});
fInstructions.push_back({BuilderOp::copy_constant, {slot}, 1, constantValue});
}

Expand Down
1 change: 0 additions & 1 deletion src/sksl/codegen/SkSLRasterPipelineBuilder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,6 @@ class Builder {
private:
void simplifyPopSlotsUnmasked(SlotRange* dst);
bool simplifyImmediateUnmaskedOp();
void simplifyOverwrittenRange(SlotRange dst);

skia_private::TArray<Instruction> fInstructions;
int fNumLabels = 0;
Expand Down
1 change: 1 addition & 0 deletions tests/sksl/folding/SelfAssignment.skrp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ copy_constant x(3) = 0
copy_4_slots_unmasked $0..3 = x
swizzle_3 $0..2 = ($0..2).zyx
copy_3_slots_unmasked x(0..2) = $0..2
splat_2_constants s.i, s.j = 0
splat_2_constants s.i, s.j = 0x40000000 (2.0)
copy_slot_unmasked s.i = s.j
copy_slot_unmasked s.j = s.i
Expand Down
1 change: 1 addition & 0 deletions tests/sksl/runtime/LargeProgram_StackDepth.skrp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
store_src_rg xy = src.rg
init_lane_masks CondMask = LoopMask = RetMask = true
splat_4_constants color = 0
splat_4_constants color = 0x3F800000 (1.0)
label label 0
copy_4_slots_unmasked $0..3 = color
Expand Down
2 changes: 2 additions & 0 deletions tests/sksl/shared/Assignment.skrp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ splat_4_constants x = 0
copy_constant x(3) = 0
splat_2_constants $0..1 = 0
swizzle_copy_2_slots_masked (x(0..1)).yx = Mask($0..1)
copy_constant ai[0] = 0
splat_4_constants ai[0], ai4[0](0..2) = 0
copy_constant ai4[0](3) = 0
copy_constant ai4[0](0) = 0x00000001 (1.401298e-45)
Expand Down Expand Up @@ -66,6 +67,7 @@ swizzle_copy_2_slots_masked (s.ah4[2]).yw = Mask($0..1)
splat_4_constants globalVar = 0
copy_constant globalStruct.f = 0
copy_constant l = 0
copy_constant l = 0
copy_2_slots_unmasked $0..1 = ai[0], ai4[0](0)
add_int $0 += $1
copy_slot_unmasked ai[0] = $0
Expand Down
2 changes: 2 additions & 0 deletions tests/sksl/shared/NumberCasts.skrp
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
store_src_rg coords = src.rg
init_lane_masks CondMask = LoopMask = RetMask = true
splat_3_constants B = 0
splat_3_constants B = 0xFFFFFFFF
splat_3_constants F = 0
copy_constant F(0) = 0x3F9D70A4 (1.23)
copy_constant F(1) = 0
copy_constant F(2) = 0x3F800000 (1.0)
splat_3_constants I = 0
splat_3_constants I = 0x00000001 (1.401298e-45)
copy_2_slots_unmasked $0..1 = F(0..1)
mul_float $0 *= $1
Expand Down
13 changes: 7 additions & 6 deletions tests/sksl/shared/ScopedSymbol.skrp
View file
Original file line number Diff line number Diff line change
@@ -1,23 +1,24 @@
store_src_rg coords = src.rg
init_lane_masks CondMask = LoopMask = RetMask = true
copy_constant glob = 0
copy_constant glob = 0x00000002 (2.802597e-45)
copy_constant _0_var = 0xFFFFFFFF
store_condition_mask $12 = CondMask
store_condition_mask $15 = CondMask
store_condition_mask $18 = CondMask
copy_constant $20 = 0xFFFFFFFF
branch_if_no_active_lanes_eq branch +6 (label 4 at #15) if no lanes of $20 == 0xFFFFFFFF
branch_if_no_lanes_active branch_if_no_lanes_active +3 (label 6 at #13)
branch_if_no_active_lanes_eq branch +6 (label 4 at #16) if no lanes of $20 == 0xFFFFFFFF
branch_if_no_lanes_active branch_if_no_lanes_active +3 (label 6 at #14)
copy_slot_unmasked $19 = glob
cmpeq_imm_int $19 = equal($19, 0x00000002)
label label 0x00000006
jump jump +3 (label 5 at #17)
jump jump +3 (label 5 at #18)
label label 0x00000004
copy_constant $19 = 0
label label 0x00000005
copy_constant $16 = 0
merge_condition_mask CondMask = $18 & $19
branch_if_no_lanes_active branch_if_no_lanes_active +5 (label 3 at #25)
branch_if_no_lanes_active branch_if_no_lanes_active +5 (label 3 at #26)
copy_constant S = 0xFFFFFFFF
copy_slot_unmasked $17 = S
label label 0x00000007
Expand All @@ -26,7 +27,7 @@ label label 0x00000003
load_condition_mask CondMask = $18
copy_constant $13 = 0
merge_condition_mask CondMask = $15 & $16
branch_if_no_lanes_active branch_if_no_lanes_active +6 (label 2 at #35)
branch_if_no_lanes_active branch_if_no_lanes_active +6 (label 2 at #36)
copy_constant S.i = 0x00000001 (1.401298e-45)
copy_slot_unmasked $14 = S.i
cmpeq_imm_int $14 = equal($14, 0x00000001)
Expand All @@ -36,7 +37,7 @@ label label 0x00000002
load_condition_mask CondMask = $15
copy_constant $0 = 0
merge_condition_mask CondMask = $12 & $13
branch_if_no_lanes_active branch_if_no_lanes_active +6 (label 1 at #45)
branch_if_no_lanes_active branch_if_no_lanes_active +6 (label 1 at #46)
copy_constant glob₁ = 0x00000001 (1.401298e-45)
copy_slot_unmasked $1 = glob₁
cmpeq_imm_int $1 = equal($1, 0x00000001)
Expand Down
9 changes: 9 additions & 0 deletions tests/sksl/shared/SwizzleConstants.skrp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@ store_src_rg coords = src.rg
init_lane_masks CondMask = LoopMask = RetMask = true
copy_4_uniforms v = testInputs
splat_3_constants v(1..3) = 0x3F800000 (1.0)
splat_2_constants v(2..3) = 0x3F800000 (1.0)
splat_3_constants v(1..3) = 0x3F800000 (1.0)
copy_constant v(0) = 0
splat_2_constants v(2..3) = 0x3F800000 (1.0)
copy_constant v(3) = 0x3F800000 (1.0)
splat_2_constants v(2..3) = 0x3F800000 (1.0)
copy_constant v(1) = 0
copy_constant v(3) = 0x3F800000 (1.0)
copy_constant v(1) = 0x3F800000 (1.0)
Expand All @@ -15,21 +19,26 @@ copy_constant v(0) = 0
splat_2_constants v(2..3) = 0x3F800000 (1.0)
splat_2_constants v(0..1) = 0x3F800000 (1.0)
copy_constant v(3) = 0x3F800000 (1.0)
copy_constant v(3) = 0x3F800000 (1.0)
copy_constant v(2) = 0
copy_constant v(2) = 0x3F800000 (1.0)
copy_constant v(3) = 0
copy_constant v(1) = 0x3F800000 (1.0)
copy_constant v(1) = 0
copy_constant v(3) = 0x3F800000 (1.0)
splat_2_constants v(1..2) = 0x3F800000 (1.0)
copy_constant v(1) = 0x3F800000 (1.0)
copy_constant v(2) = 0
copy_constant v(3) = 0x3F800000 (1.0)
copy_constant v(0) = 0x3F800000 (1.0)
copy_constant v(0) = 0
copy_constant v(3) = 0x3F800000 (1.0)
copy_constant v(0) = 0
copy_constant v(2) = 0x3F800000 (1.0)
copy_constant v(0) = 0x3F800000 (1.0)
splat_2_constants v(2..3) = 0x3F800000 (1.0)
splat_2_constants v(0..1) = 0
splat_2_constants v(0..1) = 0
copy_constant v(3) = 0x3F800000 (1.0)
copy_constant v(0) = 0
splat_2_constants v(1..2) = 0x3F800000 (1.0)
Expand Down

0 comments on commit 90e1ec7

Please sign in to comment.