Fix nondeterminism around patch constant functions - #2099
Fix nondeterminism around patch constant functions#2099Tristan Labelle (tristanlabelle) merged 2 commits into
Conversation
| if (NewPatchConstantFunc) { | ||
| // Update all user hull shaders | ||
| for (Function *HullShaderFunc : PatchConstantFuncEntry.second) | ||
| DM.SetPatchConstantFunctionForHS(HullShaderFunc, NewPatchConstantFunc); |
There was a problem hiding this comment.
Tex Riddell (@tex3d) If you can have a look at this SetPatchConstantFunctionForHS. I think it has more issues because it's using a set<Function*> to keep track of the patch constant functions, but that could break down if multiple hull shaders have the same patch constant function. This is all for supporting IsPatchConstantFunction(), and I'm wondering if it'd be so bad to implement that by iterating over all the entry points and seeing if any is a hull shader pointing to that function. It would certainly be less error-prone and less bookkeeping.
There was a problem hiding this comment.
I see what you mean. This code doesn't fix that problem though, right? What about only removing from the set on RemoveFunction. Is there a scenario where a function goes from being a patch constant function to something exported but no longer a patch constant function? I don't think so...
There was a problem hiding this comment.
No, my code doesn't fix that, though it does fix the nondeterminism. I just spotted that additional issue while looking at that code,a nd I don't know if it causes actual bugs. To be correct, we would have to also iterate over all hull shader entry points and null out the patchConstFunction pointer in that RemoveFunction. And I can see more cases where we could get the set<> and the presence of non-null patchConstFunctions out of sync, which is why I'm thinking that maintaining that set may be more pain than it's worth.
There was a problem hiding this comment.
I filed issue #2115 on it. I'd like to get this in as-is so we at least have the nondeterminism fix because fixing the tracking of patch constant functions is a bit more involved.
|
✅ Build DirectXShaderCompiler 1.0.1658 completed (commit dc163e8639 by @MrTrillian) |
|
✅ Build DirectXShaderCompiler 1.0.1692 completed (commit 0115889341 by @MrTrillian) |
|
Oops, I didn't mean to push that change... I started implementing using a counted set of |
|
Reverted the counted set change. I'd like to check this in to at least fix the nondeterminism issue as I'm not sure I'll have time to get to the patch constant function tracking issue now. Filed issue #2115 on it. |
|
✅ Build DirectXShaderCompiler 1.0.1695 completed (commit 060aa12db0 by @MrTrillian) |
3 participants
We had disabled flaky tests, and they were due to a nondeterministic pointer recycling issue.
When multiple hull shaders point to a single patch constant buffer, we would iterate over the hull shaders, during which we would apply a transformation on the patch constant buffer and erase it. This means that subsequent hull shaders will be pointing to an already-deleted patch constant buffer. If the pointer got reused by a new function, we got into weird situations were we would remove a newer function while trying to clean an invalidated pointer.
The solution is to bin hull shaders by their common patch constant function. Transform the patch constant function without deleting the original, performing the replace on every using hull shader function, and only then deleting the original function.
There are probably more things wrong with this code, like using a non-counted set to keep track of which functions are patch constant functions, but this at least fixes the nondeterminism.
Fixes #1603