Skip to content
This repository was archived by the owner on Dec 29, 2022. It is now read-only.

[spirv] temporary emission of DebugTypeMember.Offset#330

Merged
jaebaek merged 1 commit intogoogle:debug_infofrom
jaebaek:temporary_debugmembertype_offset_emit
Sep 16, 2020
Merged

[spirv] temporary emission of DebugTypeMember.Offset#330
jaebaek merged 1 commit intogoogle:debug_infofrom
jaebaek:temporary_debugmembertype_offset_emit

Conversation

@jaebaek
Copy link
Copy Markdown

@jaebaek jaebaek commented Sep 11, 2020

When the physical layout of a local variable is unknown, we cannot
properly set Offset and Size of DebugTypeMember based on the current
spec. We need the spec update.

Since we need to test SwiftShader debugger (the first Vulkan shader
debugger that can be used as a reference implementation), we want to
temporarily emit Offset and Size of DebugTypeMember even for local
variables with the unknown physical layout. We assume it uses the
tight data filling.

When the physical layout of a local variable is unknown, we cannot
properly set Offset and Size of DebugTypeMember based on the current
spec. We need the spec update.

Since we need to test SwiftShader debugger (the first Vulkan shader
debugger that can be used as a reference implementation), we want to
temporarily emit Offset and Size of DebugTypeMember even for local
variables with the unknown physical layout. We assume it uses the
tight data filling.
@jaebaek jaebaek requested a review from ehsannas September 11, 2020 15:43
@jaebaek jaebaek self-assigned this Sep 11, 2020
@AppVeyorBot
Copy link
Copy Markdown

// For example, we do not have physical layout for a local variable.

// Get offset (in bits) of this member within the composite.
uint32_t offsetInBits = field.offset.hasValue()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in what situation would field.offset not have a value?

should we assert that it has a value and then use it?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we know the physical memory layout, field.offset has a value, but it does not when we do not know the physical layout e.g., local variables.

should we assert that it has a value and then use it?

No, I think we have to use compositeSizeInBits when field.offset does not have a value e.g., local variable.

Comment thread tools/clang/lib/SPIRV/DebugTypeVisitor.cpp
Copy link
Copy Markdown
Author

@jaebaek jaebaek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your code review. PTAL!

// For example, we do not have physical layout for a local variable.

// Get offset (in bits) of this member within the composite.
uint32_t offsetInBits = field.offset.hasValue()
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we know the physical memory layout, field.offset has a value, but it does not when we do not know the physical layout e.g., local variables.

should we assert that it has a value and then use it?

No, I think we have to use compositeSizeInBits when field.offset does not have a value e.g., local variable.

Comment thread tools/clang/lib/SPIRV/DebugTypeVisitor.cpp
Comment thread tools/clang/lib/SPIRV/DebugTypeVisitor.cpp
@jaebaek jaebaek merged commit 3e9db48 into google:debug_info Sep 16, 2020
@jaebaek jaebaek deleted the temporary_debugmembertype_offset_emit branch September 16, 2020 21:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants