Skip to content

[dxil2spv] SPIR-V validation and related fixes#4328

Merged
sudonatalie merged 4 commits intomicrosoft:masterfrom
sudonatalie:validation-fixes
Mar 17, 2022
Merged

[dxil2spv] SPIR-V validation and related fixes#4328
sudonatalie merged 4 commits intomicrosoft:masterfrom
sudonatalie:validation-fixes

Conversation

@sudonatalie
Copy link
Copy Markdown
Collaborator

Add SPIR-V validation after the module is generated, and apply some
fixes required for validation to succeed with a passthrough pixel
shader:

  • Add execution mode
  • Decorate with SPIR-V Locations
  • Refactor some LowerTypeVisitor logic to a visitor that is shared with
    dxil2spv
  • Remove extra pointerness on stage IO vars now that it is handled by
    the above pass

@sudonatalie sudonatalie requested a review from jaebaek March 15, 2022 14:35
@AppVeyorBot
Copy link
Copy Markdown

@sudonatalie sudonatalie added the dxil2spv Features, bugs, tests for dxil2spv binary label Mar 15, 2022
@AppVeyorBot
Copy link
Copy Markdown

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am worried that readers cannot expect bool LowerTypeVisitor::visitInstruction(SpirvInstruction *instr) uses SpirvTypeVisitor object as a local variable.

I'd like to place this in std::vector<uint32_t> SpirvBuilder::takeModule() right after

LowerTypeVisitor lowerTypeVisitor(*astContext, context, spirvOptions);
mod->invokeVisitor(&lowerTypeVisitor);
// here

Is there any issue in that change?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This was my original plan and there shouldn't be an issue with this, but after enough time trying to detangle them I decided to go this route for some middle ground between avoiding code duplication, and going overkill on a refactoring that isn't the highest priority one for us right now. I've added a clarifying comment since I think this may be easier/more beneficial to refactor later on.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am still concerned about this because we have an implicit rule to call the visitors only in the takeModule() method of SpirvBuilder. I know it can degrade the performance because it will visit all the instructions, but doing it in the takeModule() will be better for the long term maintenance.

I also took some time to understand this part. I believe later more readers would have the same issue.

I am sorry if I bother you but I'd like to update it

LowerTypeVisitor lowerTypeVisitor(*astContext, context, spirvOptions);
mod->invokeVisitor(&lowerTypeVisitor);
SpirvTypeVisitor spvTypeVisitor(context, spirvOptions);
mod->invokeVisitor(&spvTypeVisitor);

(if it does not have technical issue).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

As discussed offline, this change doesn't work in the current state, so I've moved it to a util function as requested.

Add SPIR-V validation after the module is generated, and apply some
fixes required for validation to succeed with a passthrough pixel
shader:
- Add execution mode
- Decorate with SPIR-V Locations
- Refactor some LowerTypeVisitor logic to a visitor that is shared with
  dxil2spv
- Remove extra pointerness on stage IO vars now that it is handled by
  the above pass
@AppVeyorBot
Copy link
Copy Markdown

@AppVeyorBot
Copy link
Copy Markdown

@AppVeyorBot
Copy link
Copy Markdown

@sudonatalie sudonatalie merged commit 4958718 into microsoft:master Mar 17, 2022
@sudonatalie sudonatalie deleted the validation-fixes branch March 17, 2022 19:49
sudonatalie added a commit to sudonatalie/DirectXShaderCompiler that referenced this pull request Jun 15, 2022
Revert "[dxil2spv] Print missing runtime errors (microsoft#4481)"

This reverts commit 7f8f9ac.

Revert "[dxil2spv] Command line parsing and output file (microsoft#4454)"

This reverts commit 2e49e68.

Revert "[dxil2spv] Add additional error checking (microsoft#4440)"

This reverts commit 3c1918e.

Revert "[dxil2spv] Add FileCheck testing (microsoft#4431)"

This reverts commit d6cec44.

Revert "[dxil2spv] Translate DXIL constants to SPIR-V (microsoft#4426)"

This reverts commit f00b3c2.

Revert "[dxil2spv] Translate SV_Position to BuiltIn::Position (microsoft#4423)"

This reverts commit b292f94.

Revert "[dxil2spv] Translate extractValue and bufferStore (microsoft#4404)"

This reverts commit 1d2a68f.

Revert "[dxil2spv] Translate createHandle and bufferLoad (microsoft#4389)"

This reverts commit 316b849.

Revert "[dxil2spv] Support shl instruction (microsoft#4388)"

This reverts commit 1039a87.

Revert "[dxil2spv] Fix build warning (microsoft#4371)"

This reverts commit 7ecebb3.

Revert "[dxil2spv] Add support for dx.op.threadId (microsoft#4354)"

This reverts commit c248a1e.

Revert "[dxil2spv] Add initial compute shader support (microsoft#4345)"

This reverts commit c701ece.

Revert "[dxil2spv] Add error checking to file tests (microsoft#4344)"

This reverts commit 9f70135.

Revert "[dxil2spv] Add passthrough vertex shader and test (microsoft#4337)"

This reverts commit c99f5a5.

Revert "[dxil2spv] SPIR-V validation and related fixes (microsoft#4328)"

This reverts commit 4958718.

Revert "[dxil2spv] Implement simple entry function creation (microsoft#4323)"

This reverts commit c29a0c7.

Revert "[dxil2spv] Read file directly into llvm::MemoryBuffer (microsoft#4304)"

This reverts commit aa26bfd.

Revert "[dxil2spv] Add stage IO variables (microsoft#4271)"

This reverts commit 2d5f186.

Revert "[dxil2spv] Add CompilerInstance object (microsoft#4229)"

This reverts commit e4cf486.

Revert "[dxil2spv] Add initial testing (microsoft#4222)"

This reverts commit c3a61b1.

Revert "[dxil2spv] Construct minimal SPIR-V module (microsoft#4216)"

This reverts commit e3da12e.

Revert "[dxil2spv] Add basic dxil2spv CI test (microsoft#4209)"

This reverts commit 57d4d2b.

Revert "Add initial dxil2spv executable (microsoft#4199)"

This reverts commit c45db48.
sudonatalie added a commit that referenced this pull request Jun 16, 2022
Revert "[dxil2spv] Print missing runtime errors (#4481)"

This reverts commit 7f8f9ac.

Revert "[dxil2spv] Command line parsing and output file (#4454)"

This reverts commit 2e49e68.

Revert "[dxil2spv] Add additional error checking (#4440)"

This reverts commit 3c1918e.

Revert "[dxil2spv] Add FileCheck testing (#4431)"

This reverts commit d6cec44.

Revert "[dxil2spv] Translate DXIL constants to SPIR-V (#4426)"

This reverts commit f00b3c2.

Revert "[dxil2spv] Translate SV_Position to BuiltIn::Position (#4423)"

This reverts commit b292f94.

Revert "[dxil2spv] Translate extractValue and bufferStore (#4404)"

This reverts commit 1d2a68f.

Revert "[dxil2spv] Translate createHandle and bufferLoad (#4389)"

This reverts commit 316b849.

Revert "[dxil2spv] Support shl instruction (#4388)"

This reverts commit 1039a87.

Revert "[dxil2spv] Fix build warning (#4371)"

This reverts commit 7ecebb3.

Revert "[dxil2spv] Add support for dx.op.threadId (#4354)"

This reverts commit c248a1e.

Revert "[dxil2spv] Add initial compute shader support (#4345)"

This reverts commit c701ece.

Revert "[dxil2spv] Add error checking to file tests (#4344)"

This reverts commit 9f70135.

Revert "[dxil2spv] Add passthrough vertex shader and test (#4337)"

This reverts commit c99f5a5.

Revert "[dxil2spv] SPIR-V validation and related fixes (#4328)"

This reverts commit 4958718.

Revert "[dxil2spv] Implement simple entry function creation (#4323)"

This reverts commit c29a0c7.

Revert "[dxil2spv] Read file directly into llvm::MemoryBuffer (#4304)"

This reverts commit aa26bfd.

Revert "[dxil2spv] Add stage IO variables (#4271)"

This reverts commit 2d5f186.

Revert "[dxil2spv] Add CompilerInstance object (#4229)"

This reverts commit e4cf486.

Revert "[dxil2spv] Add initial testing (#4222)"

This reverts commit c3a61b1.

Revert "[dxil2spv] Construct minimal SPIR-V module (#4216)"

This reverts commit e3da12e.

Revert "[dxil2spv] Add basic dxil2spv CI test (#4209)"

This reverts commit 57d4d2b.

Revert "Add initial dxil2spv executable (#4199)"

This reverts commit c45db48.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dxil2spv Features, bugs, tests for dxil2spv binary

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants