Skip to content

Rewriter: improvements plus extract uniforms to global scope#2730

Merged
tex3d merged 2 commits intomicrosoft:masterfrom
tex3d:rewrite-uniform
Mar 5, 2020
Merged

Rewriter: improvements plus extract uniforms to global scope#2730
tex3d merged 2 commits intomicrosoft:masterfrom
tex3d:rewrite-uniform

Conversation

@tex3d
Copy link
Copy Markdown
Contributor

@tex3d tex3d commented Mar 2, 2020

  • Added rewriter functions for extracting uniforms into global scope
  • Will not work if name collides (namespaces have bugs)
  • Values under cbuffer _Params, resources outside (more bugs)
  • Added rewriter HLSLOptions and support all through RewriteWithOptions
  • Refactored dxr.exe to use HLSLOptions and RewriteWithOptions
  • Exposed Write*VersionInfo functions from dxclib
  • Fixed issue with External[Lib/Fn] for version printing.

- Added rewriter functions for extracting uniforms into global scope
- Will not work if name collides (namespaces have bugs)
- Values under cbuffer _Params, resources outside (more bugs)
- Added rewriter HLSLOptions and support all through RewriteWithOptions
- Refactored dxr.exe to use HLSLOptions and RewriteWithOptions
- Exposed Write*VersionInfo functions from dxclib
- Fixed issue with External[Lib/Fn] for version printing.
@tex3d tex3d requested a review from pow2clk March 2, 2020 03:17
@AppVeyorBot
Copy link
Copy Markdown

Copy link
Copy Markdown
Collaborator

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

Looks good! I just included a few suggestions you can take or leave.

Comment thread include/dxc/Support/HLSLOptions.td Outdated
} // namespace dxc

// Collects compiler/validator version info
void DxcContext::GetCompilerVersionInfo(llvm::raw_string_ostream &OS) {
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.

It's pre-existing, but Get* seems like the wrong prefix and that comment is also misleading. Nothing is gotten or collected, it's printed out to the given stream. I think adopting the prefix Write* might make this less confusing.

NamedDecl *entryDecl = l.front();
FunctionDecl *entryFnDecl = dyn_cast_or_null<FunctionDecl>(entryDecl);
if (entryFnDecl == nullptr) {
w << "//entry point found but is not a function declaration\n";
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.

Pre-existing, but for these outputs, including the name of the entry point might be helpful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure I want to get into improving this bit of code more at the moment... besides the name of the entry point was given by the user as an argument to the operation here presumably.

Copy link
Copy Markdown
Collaborator

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

Good descriptions!

@AppVeyorBot
Copy link
Copy Markdown

@tex3d tex3d merged commit ccd45ba into microsoft:master Mar 5, 2020
@tex3d tex3d deleted the rewrite-uniform branch March 5, 2020 05:27
tex3d added a commit to tex3d/DirectXShaderCompiler that referenced this pull request Mar 13, 2020
…ft#2730)

- Added rewriter functions for extracting uniforms into global scope
- Will not work if name collides (namespaces have bugs)
- Values under cbuffer _Params, resources outside (more bugs)
- Added rewriter HLSLOptions and support all through RewriteWithOptions
- Refactored dxr.exe to use HLSLOptions and RewriteWithOptions
- Exposed Write*VersionInfo functions from dxclib
- Fixed issue with External[Lib/Fn] for version printing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants