-
Notifications
You must be signed in to change notification settings - Fork 245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve the calculation of location sizes for arrays and structs (revives #513) #798
base: main
Are you sure you want to change the base?
Conversation
|
So I still get validation and perf warning for the specific use case I wanted this for, but I can't do anything about that :P
|
…gpu into revive-array-location-fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo the name of the new method.
@@ -554,4 +554,38 @@ impl<'tcx> CodegenCx<'tcx> { | |||
} | |||
} | |||
} | |||
|
|||
fn location_size_of_type(&self, ty: Word) -> u32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be renamed to location_count_of_type
or location_slots_per_type
? "size_of" could get confused with the size in bytes, which AFAICT is not relevant here.
// normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" | ||
// normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" | ||
// normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" | ||
// normalize-stderr-test "OpMemberName %12 0 .0.\n" -> "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's up with this rewrite rule? It seems like it's probably hiding a bug that should be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is here because the ordering of the OpMemberName
s gets altered between runs:
running 1 test
diff of stderr:
OpCapability Float64
OpCapability Int16
OpCapability Int64
OpCapability Int8
OpCapability ShaderClockKHR
OpCapability Shader
OpExtension "SPV_KHR_shader_clock"
OpMemoryModel Logical Simple
OpEntryPoint Fragment %1 "main" %2 %3 %4 %5 %6 %7 %8 %9
OpExecutionMode %1 OriginUpperLeft
%10 = OpString "$OPSTRING_FILENAME/array_location_calculation.rs"
OpMemberName %11 0 "x_axis"
OpMemberName %11 1 "y_axis"
OpMemberName %11 2 "z_axis"
OpName %11 "spirv_std::glam::core::storage::Columns3<spirv_std::glam::XYZ<f32>>"
OpMemberName %12 0 "0"
OpName %12 "spirv_std::glam::Mat3"
OpName %13 "array_location_calculation::main"
OpName %2 "one"
OpName %3 "two"
OpName %4 "three"
OpName %5 "four"
OpName %6 "five"
OpName %7 "six"
OpName %8 "seven"
OpName %9 "eight"
OpMemberName %11 0 "x_axis"
OpMemberName %11 1 "y_axis"
OpMemberName %11 2 "z_axis"
-OpMemberName %12 0 "0"
-OpMemberName %12 0 "0"
OpMemberName %11 0 "x_axis"
OpMemberName %11 1 "y_axis"
OpMemberName %11 2 "z_axis"
+OpMemberName %12 0 "0"
+OpMemberName %12 0 "0"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, that seems like a bug.
Totally forgot about this - I need to check whether it's still needed. |
Ah, didn't see this when I set the labels, I'll flip them. |
See PR #513 that @Arc-blroth opened.
This PR builds on top of that to support structs and 3 or 4 component 64-bit scalar type vectors (see #513 (review)).