Repository navigation
fix(agc): lower hull exec masks restored through phis to own lane bits - #4172
oneandonlydean merged 1 commit into
Conversation
bf8a19f to
5666086
Compare
|
I force-pushed this (and restacked #4201 and #4222 on it) after a Yōtei run found a problem the first version caused. The own-lane-bit rewrite also ran in domain and local programs, and there it turned The rewrite now runs in hull programs only, which are the only ones that need it. The terrain hull's SPIR-V is byte-identical, and Checks after the change
|
|
Rebase handoff — rebased on top of the rebased chain ( One test-file conflict with main's newer Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release,
To pick it up (pull the chain in order): Heads: |
5666086 to
bad2835
Compare
bad2835 to
268bdda
Compare
268bdda to
da0e620
Compare
|
Resident Evil Village (PPSA01556) hit this in a domain program through the old Fix and test, on #4222's headdiff --git a/core/libs/prx/libSceAgcDriver/tests/execution/Tessellation.cpp b/core/libs/prx/libSceAgcDriver/tests/execution/Tessellation.cpp
index 79c98df98..461063765 100644
--- a/core/libs/prx/libSceAgcDriver/tests/execution/Tessellation.cpp
+++ b/core/libs/prx/libSceAgcDriver/tests/execution/Tessellation.cpp
@@ -51,6 +51,16 @@ alignas(256) constexpr std::array<std::uint32_t, 49> HullCode{
0xbf810000,
};
+alignas(256) constexpr std::array<std::uint32_t, 54> HullCompareBranch{
+ 0xd4040016, 0x00020080, 0xbeea0416, 0xbf870001, 0xbf800000, 0xd7650000, 0x000100c1, 0xd7660000,
+ 0x000200c1, 0xbf8a0000, 0x7da206f9, 0x81060000, 0xd5480014, 0x02151101, 0x362402ff, 0x000000ff,
+ 0x160028a0, 0xd5430015, 0x040224ff, 0x00000060, 0xdbfc0000, 0x02000015, 0x162c2890, 0xd5430016,
+ 0x045a24ff, 0x00000030, 0xbf8cc07f, 0xe0781000, 0x02020216, 0x7da82881, 0xbe94047e, 0xb06a0060,
+ 0x162602f9, 0x0086066a, 0xd8d80008, 0x06000013, 0xd8dc1008, 0x07000013, 0xbf8cc07f, 0x7e300306,
+ 0x7e320306, 0x7e340306, 0x7e360306, 0x34142484, 0xbefe0414, 0xe0781000, 0x0403180a, 0x4a162481,
+ 0x16161690, 0x4c1616ff, 0x00008000, 0xe0741000, 0x0202070b, 0xbf810000,
+};
+
alignas(256) constexpr std::array<std::uint32_t, 51> HullRestoredExecSaved{
0xd7650000, 0x000100c1, 0xd7660000, 0x000200c1, 0xbf8a0000, 0x7da206f9, 0x81060000, 0xd5480014,
0x02151101, 0x362402ff, 0x000000ff, 0x160028a0, 0xd5430015, 0x040224ff, 0x00000060, 0xdbfc0000,
@@ -76,6 +86,14 @@ alignas(256) constexpr std::array<std::uint32_t, 34> DomainRelativeData{
0x14151214, 0xbf810000,
};
+alignas(256) constexpr std::array<std::uint32_t, 35> DomainCompareBranch{
+ 0xf8000941, 0x00000000, 0xd4040014, 0x00020a80, 0xbeea0414, 0xbf870001, 0xbf800000, 0x34020e84,
+ 0x16020283, 0xe0341000, 0x04030a01, 0xe0341010, 0x04030c01, 0xe0341020, 0x04030e01, 0xbf8c3f70,
+ 0x08040af2, 0x08040d02, 0x10201505, 0x3e201906, 0x3e201d02, 0x10221705, 0x3e221b06, 0x3e221f02,
+ 0xd5510012, 0x040a0d05, 0x102424ff, 0x40400000, 0x7e260280, 0x7e2802f2, 0xf80008cf, 0x14131110,
+ 0xf800020f, 0x14131214, 0xbf810000,
+};
+
alignas(256) constexpr std::array<std::uint32_t, 29> LocalIndexAsData{
0xd5480004, 0x02090502, 0xd5480005, 0x02090102, 0xd5480006, 0x02110902, 0xd5480007, 0x02111102,
0x7e080d04, 0x7e0a0d05, 0x7e0c0d06, 0x7e0e0d07, 0x10100cff, 0x3f19999a, 0x061010ff, 0xbf666666,
@@ -357,6 +375,12 @@ void ExpectRefusal(const ShaderRecompiler::SpirvTarget& target, ShaderStage stag
Require(false, "a tessellation program was accepted although " + reason);
}
+void RequireCompareBranches(const ShaderRecompiler::SpirvTarget& target) {
+ std::uint32_t cursor = 0;
+ Require(!Compile(target, ShaderStage::TessellationEvaluation, DomainCompareBranch, cursor).spirv.empty(), "a domain program branching on a compare copied to VCC was not compiled");
+ Require(!Compile(target, ShaderStage::TessellationControl, HullCompareBranch, cursor).spirv.empty(), "a hull program branching on a compare copied to VCC was not compiled");
+}
+
std::array<std::uint8_t, 4> PixelAt(std::uint32_t x, std::uint32_t y) {
const auto offset = (static_cast<std::size_t>(y) * Width + x) * 4u;
return {std::to_integer<std::uint8_t>(Pixels[offset]), std::to_integer<std::uint8_t>(Pixels[offset + 1]), std::to_integer<std::uint8_t>(Pixels[offset + 2]), std::to_integer<std::uint8_t>(Pixels[offset + 3])};
@@ -700,6 +724,7 @@ int main() {
RequirePatchConstants(*device, target);
RequireRelativePatches(*device, target);
RequireHullBdaReads(*device, target);
+ RequireCompareBranches(target);
device.reset();
RequireRegisteredStages();
std::puts("Tessellation tests passed");
diff --git a/core/shader/recompiler/Optimization/src/TessellationLowering.cpp b/core/shader/recompiler/Optimization/src/TessellationLowering.cpp
index 6028333e9..8d9e91c42 100644
--- a/core/shader/recompiler/Optimization/src/TessellationLowering.cpp
+++ b/core/shader/recompiler/Optimization/src/TessellationLowering.cpp
@@ -588,9 +588,12 @@ private:
for (IrValue* inst : block->Instructions()) {
const auto words = LaneBitWords(inst, program.WaveSize());
if (!words) continue;
- if (IrValue* bit = laneBits.Of(*words)) inst->ReplaceAllUsesWith(bit);
+ if (IrValue* bit = laneBits.Of(*words)) inst->ReplaceUsesWith(bit, true);
}
}
+ ResolveControlFlowIdentities(program);
+ constexpr DeadCodeEliminator identities;
+ identities.RemoveIdentities(program);
}
void collect() {Village also needs two tessellation changes your chain doesn't have:
All three are rebased on #4222 with tests, red/green on Windows, and passing on Linux. With them, 19 of Village's 21 dumped tessellation programs compile on your chain. The other 2 only miss #4114's patch bound, because the dumped requests carry no NUM_PATCHES; at 128 or 170 patches all 21 compile. How would you like them? I could open them as PRs that depend on #4222, you could take the |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the approach looks right. One blocker:
lowerOwnLaneBits(TessellationLowering.cpp:584) usesReplaceAllUsesWith(bit), butBlockInfo::condition(set inExecMask.cpp:206) is a plain pointer that the replacement doesn't touch. A hull that branches on a compare copied to VCC (v_cmp_* s[n:n+1],s_mov_b64 vcc, s[n:n+1],s_cbranch_vccnz) keeps its block condition on the replaced value, so after dead-code elimination SPIR-V emission fails withbranch condition ... was not emitted before the branch. @EagleBearerYS reported this above with a hull test and a fix that replaces through an identity, as the front end does. Please take that fix and itsHullCompareBranchtest (adjusted to this head), and check that the test fails without the change.
Everything else I read looks consistent with the description: the half-wave matcher is untouched for local/domain programs and HullLaneCount is still refused. agc_shader_memory, agc_driver_graphics and agc_tessellation_factor_ring pass here; agc_driver_tessellation passes on CI's lavapipe.
da0e620 to
8cbf91d
Compare
|
@adriadam10 @EagleBearerYS I've folded the block-condition fix into this PR's commit. The I also rebased onto current DetailsNew heads: #4172 Without the fix (this head with only the With the fix, Windows 11 on an RTX 3080 Laptop GPU (driver 610.62):
Linux (upstream's job in WSL, GCC 13.3, llvmpipe), #4222's head merged with |
|
Thanks for folding the block-condition fix and its test into this commit, and for the credit. I've rebased #5457 onto #4222's new head (f33872b). Its two commits are unchanged apart from one test-array conflict, and its head is now 5589f92. On my fork's full Build, with that branch merged into Yes please on Yōtei's hulls, and thanks for offering. |
A wave64 hull program narrows EXEC with s_and_saveexec_b64 and restores it later, so its exec words reach the lane-bit test as ANDs with the host ballot, as loop or join phis, and through separate ballots for the two halves. Tessellation lowering refused them as "uses a lane mask other than its own lane's bit". The own-lane bit of a mask pair is now rebuilt from the words' operations (ballots, bitwise ops, selects, phis, constants); both halves must have the same shape and ballot the same predicate. Dead hull cycles such as a saved-exec loop are removed before the remaining-ballot check, so a mask whose other lanes are read (s_bcnt1_i32_b64 exec) is still refused. The words are replaced through identities and block conditions are resolved, so a branch on a compare copied to VCC keeps its condition. Local and domain programs keep their exec words for the half-wave matcher. Block conditions fix and HullCompareBranch test from EagleBearerYS.
8cbf91d to
866ed44
Compare
Met on 866ed44: every point of this review is addressed; full build green on this head and Sarah passes.
What
In a hull, each host invocation is one guest lane, so a lane mask can only be kept if every use reads the invocation's own bit. Tessellation lowering recognised that for a ballot-derived saved exec, but a wave64 hull that narrows EXEC with
s_and_saveexec_b64and restores it through phis (also inside a scalar loop) was refused as "uses a lane mask other than its own lane's bit".SelectU32and phis boolean selects and phis (loops included), all-zero or all-one words constants. Both halves must have the same shape and ballot the same predicate; wave32 uses the low word only.s_mov_b64 vcc,s_cbranch_vccnz) otherwise kept its block condition on the replaced word and failed SPIR-V emission. Fix and test by @EagleBearerYS.PPSA26344's first terrain hull (0x80002fc700) restores its exec this way; with this change its replayed request gets past the lane-mask refusal.
#5457 builds on this stack and edits the same lowering and test file.
Tested
Based on
mainf265cd5. Windows 11 (MinGW-w64 GCC 15.2.0) and Linux: configure -DBUILD_TESTING=ON, targeted test builds, ctest:agc_driver_tessellation,agc_shader_memory,agc_driver_graphicsandagc_tessellation_factor_ringpass (4/4); on fix(shader): record BDA faults in hull programs instead of refusing them #4222's head, withagc_driver_bda_device, 5/5main: the same 5 tests pass (5/5)log_macros, which fails onmaintoo; Linux (upstream's job in WSL) merged withmain5a81f60: 1089/1091,agc_driver_bc_block_copyandagc_driver_depth_compare_copyfail on thatmainas wellIn
agc_driver_tessellation,HullAndSaveexecis now a rendered check,HullSaveexecLoop(exec restored through loop phis, with a dead mask accumulator) is new and rendered,HullLaneCount(s_bcnt1_i32_b64 execas data) is still refused, andRequireCompareBranchescompiles a hull and a domain that branch on a compare copied to VCC.agc_shader_memorychecks that domain wave reductions unders_orn2_saveexec_b64still reduce on 32 and 64 lanes. They fail onmainwith the lane-mask refusal; without the identity replacement the compare-branch hull fails with "branch condition ... was not emitted before the branch".Not tested: console behaviour.
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)