shader_recompiler: split resource tracking and flatten load from buffer for sharp source - #4782
shader_recompiler: split resource tracking and flatten load from buffer for sharp source#4782LNDF wants to merge 44 commits into
Conversation
| @@ -185,7 +202,7 @@ static void VisitPointer(u32 off_dw, IR::Inst* subtree, PassInfo& pass_info, | |||
| c.mov(r10d, ptr[rdi + (src_off_dw << 2)]); | |||
There was a problem hiding this comment.
Do i need to handle buffer stride etc here if ReadConstBuffer?
|
On this PR, Call of Duty Cold War crashes in roughly the same point with this new critical |
|
Killzone Shadow Fall still crashes while loading into offline match, now crashing with |
|
Skyrim immediately crashes with this PR |
|
Current push fixes the Skyrim regression. Now the game behaves like main again. |
Should be fixed
Try latest commit
Try latest commit |
It is fixed for me |
|
This PR causes regressions in Uncharted: The Nathan Drake Collection and Bloodborne. I've reported these directly to @LNDF, just mentioning them here so people know these issues are already known. |
|
Far Cry 5 shows this on this PR Main build crashes on both cases without even showing a logo Final Fantasy XV crashes with Main |
|
COD Cold War now hangs rather than crashing. |
|
|
||
| static bool EmitComputeOffsetIAdd32(Xbyak::CodeGenerator& c, Xbyak::Reg32 reg, PassInfo& pass_info, | ||
| IR::Inst* inst) { | ||
| if (inst->AreAllArgsImmediates()) { |
There was a problem hiding this comment.
I feel this check might not be needed as constant propagation has already run before this and should have constant folded the operation, have you noticed this in some code?
There was a problem hiding this comment.
Done.
Tho min and max afaik are not constant folded so i left them
| u32 vn; | ||
|
|
||
| switch (inst->GetOpcode()) { | ||
| case IR::Opcode::Phi: { |
There was a problem hiding this comment.
I feel this also suffers from the logic bug in main. If a shader has something like that
addr = ...
read_const(addr)
...
addr += 4
read_const(addr)
then the GVN might consider them identical since it skips over the intermediate add sees same addr. I have not confirmed this so worth checking. Another question is if we really need GVN pass, how much de-duplication actually happens in practice
There was a problem hiding this comment.
For other insts that are not GetUserData, CompositeConstructU32x2, ReadConst and ReadConstBuffer GVN will associate an incrementing number to the instruction itself. For those 4 instructions it will insteed assign an incrementing number to a hash of the numbers given to the argument instructions.
Because of this, if you have
%10 = ReadConst %9 ...
%11 = IAdd32 %10 ...
%12 = ReadConst %11 ...
Both readconsts should have different number since the argument is different.
I figured out this by reading the gvn table .h file so i might have understood something wrong. Is there a game that is affected by this in main?
There was a problem hiding this comment.
I think the example given was incorrect, what about this
addr = user_data()
if (cond0)
addr0 = addr + 4;
else
addr0 = addr;
read_const(addr0);
if (cond1)
addr1 = addr + 8
else
addr1 = addr;
read_const(addr1);
Each ReadConst is gonna have a phi as address where 1 argument is the same SSA value directly from user data and another is an IAdd. The bfs is gonna return the addr SSA in both cases resulting in same value number
There was a problem hiding this comment.
Done. We now handle phis as any other inst (so assugn a new gvn value for each phi)
There was a problem hiding this comment.
Actualy, just updated it to use the same logic as in flattening pass
|
|
||
| // Perform dominance analysis on findings and eliminate ones that don't pass | ||
| // If a finding is dominated by another, the former can be eliminated. | ||
| size_t num_findings = findings.size(); |
There was a problem hiding this comment.
I think this should be a separate function in dominance.h that takes the findings array by reference and prunes it
There was a problem hiding this comment.
Yes. But is dominance.h the correct place? That is for frontend and what we are trying to separate is ir stuff.
There was a problem hiding this comment.
Well this code prunes sources based on dominance analysis I feel it matches better in dominance than a what is supposed to be a generic BFS algorithm header impl
| } | ||
|
|
||
| template <typename Instruction, typename Pred> | ||
| auto DominatingBreadthFirstSearch(Instruction* inst, const IR::Block& current_parent, bool deep, |
There was a problem hiding this comment.
The BFS header is getting crowded so worth doing a more holistic review.
The normal BFS
- Only finds 1 instruction its looking for and stops
- Keeps searching until it finds it without any way to stop it
This BFS
- Can find multiple sources if presents
- Is more selective where it searches and deep option stops it from going thru phis
A review of all BFS usages in recompiler could be useful to deduce if its possible to make a unified function that handles everything (also could use std::expected as predicate return to allow user to specify if they want to stop search on the current path or entirely etc)
| } | ||
|
|
||
| boost::container::small_vector<IR::Inst*, 8> visited, findings; | ||
| std::queue<IR::Inst*> queue; |
There was a problem hiding this comment.
Small optimization; since we spam this function a lot in resource discover might be useful to either pass this as argument in function so resource discover can maintain a queue or make this static and clear it on function entry, to avoid reconstructing the object on every call and associated heap allocations
| // For inmediates, add a sharp usage with null sharp source. | ||
| sharp_usages.emplace_back(ResourceDiscovery{&inst, &block, nullptr}); | ||
| } else { | ||
| IR::Inst* buffer_handle = handle->Arg(0).InstRecursive(); |
There was a problem hiding this comment.
For buffers handle is a composite that contains all 4 dwords of V#, can just call FindSharpSource for each, store them in an array (as mentioned in other review comment) and in resource patching verify offsets of flattened instructions are contiguous. If they are not, additional logic could be introduced (for example using gather intrinsic like _mm_i32gather_epi32 with a slim abstraction could be suitable)
There was a problem hiding this comment.
Do you think this is in scope for this PR or out of scope?
IMO its a better idea to do this in another PR where we also include the refactor of image instructions to include both the hish and low vectors.
There was a problem hiding this comment.
Do you think this is in scope for this PR or out of scope?
IMO its a better idea to do this in another PR where we also include the refactor of image instructions to include both the hish and low vectors.
There was a problem hiding this comment.
Sure it could be possible. You should add an assertion at least that the ReadConstBuffer insts you find have a use in the composite handle of the buffer opcode
|
|
||
| if (sharp_source && sharp_source->GetOpcode() == IR::Opcode::ReadConstBuffer) { | ||
| const auto texture_flags = inst.Flags<IR::TextureInstInfo>(); | ||
| const auto is_r128 = texture_flags.is_r128.Value(); |
There was a problem hiding this comment.
Mentioning here too, image sample ir op should be updated to hold 2 composites for the low and high part of the T# and do the same as buffers. The r128 check should be entirely a frontend thing and leave the high composite null
There was a problem hiding this comment.
Answered in the previous comment
| auto inst_info = inst->Flags<IR::BufferInstInfo>(); | ||
| return inst_info.flatbuf_off_dw; | ||
| } | ||
| return inst->Flags<u16>(); |
There was a problem hiding this comment.
for good measure check if its IR::Opcode::ReadConst and add an UNREACHABLE at the end
| if (!inst.Arg(1).IsImmediate()) { | ||
| LOG_WARNING(Render_Recompiler, "ReadConst has non-immediate offset"); | ||
| continue; | ||
| if (inst.GetOpcode() == IR::Opcode::ReadConst || |
There was a problem hiding this comment.
condition can be inverted to reduce nesting
| IR::Inst* inst{queue.front()}; | ||
| queue.pop(); | ||
|
|
||
| if (inst->GetOpcode() == IR::Opcode::ReadConstBuffer) { |
There was a problem hiding this comment.
Should this stop when it encounters a Phi? iirc the dynamic offset emit cannot handle them atm though not sure what the plan is to add them in the future. Also this is another BFS variant that searches the entire tree without stopping, would be good to unify it with the rest
There was a problem hiding this comment.
Currently will go through all possible previous blocks. Then this will log an error when generating the x64 code.
This comment was marked as abuse.
This comment was marked as abuse.
Thanks for the report. This regression is already know and will be delt with before merging. There are more games thatare affected. First will apply the suggested changes on the review though. |
This splits resource tracking pass into resource discovery and patching passes. Then we put flattening pass in the middle.
That way: