Vulkan: Fix wrong descriptor set indices when binding descriptor sets #2191
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes the wrong calculation of descriptor set indices when binding descriptor sets. Also, for multi-layer maps (e.g. `map!(u32, map!(u32...`), we need to explicitly assign the child map back to the parent map. This is because the generated go/c++ code will not assign the child map back to the parent map automactically (go: `Get()`, c++: `findOrZero`) when write api file like: `parent[parent_key][child_key] = child_content`. TODO: Similar bugs for other mutlti-layer maps
Qining
commented
Sep 11, 2018
gapil/analysis/reference_value.go
Outdated
| @@ -171,8 +171,10 @@ func (v *ReferenceValue) Clone() Value { | |||
| func (v *ReferenceValue) field(s *scope, name string) Value { | |||
| candidates := make([]Value, 0, len(v.Assignments)) | |||
| for a := range v.Assignments { | |||
| field := s.getInstance(a).(fieldHolder).field(s, name) | |||
| candidates = append(candidates, field) | |||
| if fh, ok := s.getInstance(a).(fieldHolder); ok { | |||
There was a problem hiding this comment.
This is changed because of build failure with the original code.
The build fails with the original code if we drop the line 721 to line 745 in gapis/api/vulkan/api/descriptor.api, without any other changes. This is weird.
The build does not happen before at commit: 7915ae3, which is the last commit that drops the line 721 to line 745 in gapis/api/vulkan/api/descriptor.api
There was a problem hiding this comment.
I'm going to try and reproduce this error - I don't think there should be a type panic here...
There was a problem hiding this comment.
Okay, after much beard scratching, I think I know what's going on here. See my comment below.
Qining
commented
Sep 11, 2018
gapil/analysis/reference_value.go
Outdated
| @@ -184,7 +186,9 @@ func (v *ReferenceValue) field(s *scope, name string) Value { | |||
| // in the scope's instances map. | |||
| func (v *ReferenceValue) setField(s *scope, name string, val Value) Value { | |||
| for a := range v.Assignments { | |||
| s.instances[a] = s.getInstance(a).(fieldHolder).setField(s, name, val) | |||
| if fh, ok := s.getInstance(a).(fieldHolder); ok { | |||
There was a problem hiding this comment.
Same problem as above
There was a problem hiding this comment.
Your fix is likely fine, however please add the below comment so that some poor bugger doesn't have to work this out again.
I suggest adding this new utility function, and using it in the two places you've changed:
func (v *ReferenceValue) fhAssignments() []fieldHolder {
out := make([]fieldHolder, 0, len(v.Assignments))
for a := range v.Assignments {
// Check the assignment has an instance. No instance can happen when the
// assignment took place in a sibling block which has not been merged
// into the common scope yet.
// In this particular case, the instance is inaccessible to this block,
// so skipping it is the correct thing to do.
if fh, ok := s.getInstance(a).(fieldHolder); ok {
out = append(out, fh)
}
}
return out
}
ben-clayton
approved these changes
Sep 13, 2018
gapil/analysis/reference_value.go
Outdated
| @@ -171,8 +171,10 @@ func (v *ReferenceValue) Clone() Value { | |||
| func (v *ReferenceValue) field(s *scope, name string) Value { | |||
| candidates := make([]Value, 0, len(v.Assignments)) | |||
| for a := range v.Assignments { | |||
| field := s.getInstance(a).(fieldHolder).field(s, name) | |||
| candidates = append(candidates, field) | |||
| if fh, ok := s.getInstance(a).(fieldHolder); ok { | |||
There was a problem hiding this comment.
Okay, after much beard scratching, I think I know what's going on here. See my comment below.
gapil/analysis/reference_value.go
Outdated
| @@ -184,7 +186,9 @@ func (v *ReferenceValue) field(s *scope, name string) Value { | |||
| // in the scope's instances map. | |||
| func (v *ReferenceValue) setField(s *scope, name string, val Value) Value { | |||
| for a := range v.Assignments { | |||
| s.instances[a] = s.getInstance(a).(fieldHolder).setField(s, name, val) | |||
| if fh, ok := s.getInstance(a).(fieldHolder); ok { | |||
There was a problem hiding this comment.
Your fix is likely fine, however please add the below comment so that some poor bugger doesn't have to work this out again.
I suggest adding this new utility function, and using it in the two places you've changed:
func (v *ReferenceValue) fhAssignments() []fieldHolder {
out := make([]fieldHolder, 0, len(v.Assignments))
for a := range v.Assignments {
// Check the assignment has an instance. No instance can happen when the
// assignment took place in a sibling block which has not been merged
// into the common scope yet.
// In this particular case, the instance is inaccessible to this block,
// so skipping it is the correct thing to do.
if fh, ok := s.getInstance(a).(fieldHolder); ok {
out = append(out, fh)
}
}
return out
}
AWoloszyn
added a commit
to AWoloszyn/gapid
that referenced
this pull request
Sep 14, 2018
…tor sets (google#2191)" This reverts commit ac65ff9.
Qining
added a commit
to Qining/gapid
that referenced
this pull request
Sep 17, 2018
This re-fixes the wrong calculation of descriptor set indices when binding descriptor sets. The fix was reverted as google#2191 get reverted. Also, for multi-layer maps (e.g. `map!(u32, map!(u32...`), we need to explicitly assign the child map back to the parent map. This is because the generated go/c++ code will not assign the child map back to the parent map automactically (go: `Get()`, c++: `findOrZero`) when write api file like: `parent[parent_key][child_key] = child_content`. TODO: Similar bugs for other mutlti-layer maps
Qining
added a commit
that referenced
this pull request
Sep 18, 2018
…ets (#2224) This re-fixes the wrong calculation of descriptor set indices when binding descriptor sets. The fix was reverted as #2191 get reverted. Also, for multi-layer maps (e.g. `map!(u32, map!(u32...`), we need to explicitly assign the child map back to the parent map. This is because the generated go/c++ code will not assign the child map back to the parent map automactically (go: `Get()`, c++: `findOrZero`) when write api file like: `parent[parent_key][child_key] = child_content`. TODO: Similar bugs for other mutlti-layer maps
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the wrong calculation of descriptor set indices when binding
descriptor sets.
Also, for multi-layer maps (e.g.
map!(u32, map!(u32...), we need toexplicitly assign the child map back to the parent map. This is because
the generated go/c++ code will not assign the child map back to the
parent map automactically (go:
Get(), c++:findOrZero) when writeapi file like:
parent[parent_key][child_key] = child_content.TODO: Similar bugs for other mutlti-layer maps