Instrument: Fix bindless checking for BufferDeviceAddress #5049
Conversation
jeremyg-lunarg
force-pushed
the
jeremyg-opconstant-null
branch
2 times, most recently
from
6577089 to
9c2151a
Compare
|
This change has been confirmed to fix the linked Vulkan-ValidationLayers bug |
There was a problem hiding this comment.
Seems ok to me but I'm not comfortable enough as a code owner.
Would like @greg-lunarg or @s-perron to formally approve
@dneto0 I will review.. |
| std::unique_ptr<Instruction> cap_int64_inst( | ||
| new Instruction(context(), spv::Op::OpCapability, 0, 0, | ||
| std::initializer_list<Operand>{ | ||
| {SPV_OPERAND_TYPE_CAPABILITY, | ||
| {uint32_t(spv::Capability::Int64)}}})); | ||
| get_def_use_mgr()->AnalyzeInstDefUse(&*cap_int64_inst); | ||
| context()->AddCapability(std::move(cap_int64_inst)); |
There was a problem hiding this comment.
I think you could replace these 7 lines with: context()->AddCapability(spv::Capability::Int64).
There was a problem hiding this comment.
IRContext::AddCapability() does the HasCapability() check internally so it doesn't need to be checked by the caller. I also fixed the other 2 places where the instrumentation code adds capabilities, one of which is for the same problem I'm fixing here.
Avoid using OpConstantNull with types that do not allow it. Update existing tests for slight changes in code generation. Add new tests based on the Vulkan Validation layer test case that exposed this problem.
jeremyg-lunarg
force-pushed
the
jeremyg-opconstant-null
branch
from
9c2151a to
690cce2
Compare
jeremyg-lunarg
requested review from
greg-lunarg
and removed request for
s-perron
|
Not all of the CIs ran. If they all pass, we can merge it. |
Is that the purpose of the kokoro:run label you added 4 days ago? Or is there something I need to do to trigger CI? |
|
The label should be good enough, but the bots do not seem to be responding. I'm looking into it now. |
|
The bots where still looking for PRs to the master branch instead of main. I've fixed that up. I'll give it a little time, so see if it worked. |
Pulls in a bugfix SPIRV-Tools issue KhronosGroup/SPIRV-Tools#5049
Avoid using OpConstantNull with types that do not allow it.
Update existing tests for slight changes in code generation.
Add new tests based on the Vulkan Validation layer test case that exposed this problem.
This is a fix for KhronosGroup/Vulkan-ValidationLayers#4983