Add --no-link option to only compile (not link) individual shaders and export all functions #3319

ncesario-lunarg · 2023-08-24T02:59:26Z

After internal discussion, we will I'm changing this slightly such that --no-link will export all functions, and the ability to export/import individual functions/variables with an attribute will be delayed until such an attribute is standardized.

[[export]]/[[import]] attribute proposal (on hold)

This change experiments with adding a "compile-only" option to glslang as well as an `[[export]]` function attribute allowing functions to be exported and then later used by spirv-link.

Proposed usage:

glslang -V --no-link myfuncs.comp -o myfuncs.spv

Where myfuncs.comp looks something like

#version 450

[[export]]
int foo(...) { ... }

[[export]]
float bar(...) { ... }

And where "foo" and "bar" will get the "Export" linkage decoration in the resulting spir-v.

To be more "complete" this change should also:

Add an [[import]] attribute for importing functions from another spir-v module
Allow variables to be exported and imported in addition to functions (ideally using the same [[export]]/[[import]] attributes)
Specify additional spir-v binaries at link time

However, in its current (functional) form, this change could benefit the ongoing improvements to the GPU-AV workflow in Vulkan-ValidationLayers. Before spending anymore time on this, I'm hoping to get some feedback on the overall approach here. I know [[export]]/[[import]] are not official attributes and that may not be the best mechanism for achieving the end goal here (e.g., just exporting all functions when in compile-only mode would work for the purposes of GPU-AV).

ncesario-lunarg · 2023-09-06T15:17:37Z

FYI @jeremyg-lunarg.

jeremyg-lunarg · 2023-09-06T15:38:54Z

I'll update my code this morning and make sure it works. I don't anticipate this change being a problem. Thanks!

jeremyg-lunarg · 2023-09-06T16:44:06Z

Works for me locally! I pushed my updated VVL branch to depend on the newest commit.

arcady-lunarg

Just a few minor questions and comments, but on the whole this looks good to me.

arcady-lunarg · 2023-09-06T23:46:06Z

glslang/HLSL/hlslAttributes.cpp

@@ -142,6 +142,8 @@ namespace glslang {
            return EatUnroll;
        else if (name == "loop")
            return EatLoop;
+        else if (name == "export")


Should this be spv::export or something?

It can. Though, wouldn't it make sense for all of the other strings in here to be constants as well then?

What I meant is that you should put it under the if (nameSpace == "spv") block so that in the HLSL, the attribute is spelled [spv::export].

ah, yes, I should have put it there.

arcady-lunarg · 2023-09-11T23:04:16Z

SPIRV/GlslangToSpv.cpp

@@ -4329,6 +4340,15 @@ spv::Id TGlslangToSpvTraverser::convertGlslangToSpvType(const glslang::TType& ty
    return convertGlslangToSpvType(type, getExplicitLayout(type), type.getQualifier(), false, forwardReferenceOnly);
 }

+spv::LinkageType TGlslangToSpvTraverser::convertGlslangToSpv(glslang::TLinkType linkType) {


Could you give this function a better name? Given we have GlslangToSpv, convertGlslangToSpv sounds very confusing. Maybe something like glslangToSpvLinkageType?

Yes, my original intent was to merge this with one of the existing convertGlslangToSpvType, but hadn't found a good way to do that and forgot to circle back. I'll rename it for now.

Does convertGlslangLinkageToSpv sound ok?

Yeah, that seems like a cromulent name.

arcady-lunarg · 2023-09-11T23:08:56Z

SPIRV/SpvBuilder.cpp

@@ -2223,6 +2245,12 @@ void Builder::enterFunction(Function const* function)
        defInst->addIdOperand(funcId);
        buildPoint->addInstruction(std::unique_ptr<Instruction>(defInst));
    }
+
+    if (auto linkType = function->getLinkType(); linkType != LinkageTypeMax) {


I don't know that we have a rule against multiple statements in an if condition, but this is the only place in the codebase such a construct appears. I would prefer you hoist the linkType declaration out.

I can do that if that is the preferred style. In this case, linkType is only intended to be used within the scope of the if block here, so using the c++17 "init-statement" went beyond just "style" here IMHO (I do realize we don't use this ubiquitously, presumably since most of the codebase is written pre-c++17).

arcady-lunarg · 2023-09-11T23:14:56Z

Test/baseResults/spv.exportFunctions.comp.out

+                              Name 5  "a"
+                              Name 6  "b"
+                              Name 11  "foo("
+                              Decorate 7(add(f1;f1;) Linkage Attributes 6579297 0


That random constant in there is apparently the name of the exported symbol, unfortunate that it gets printed in such an unreadable way but I don't think it's worth fixing in this change.

Yeah, this is really unreadable. The spirv-dis output makes a lot more sense:

... OpName %7 "add(f1;f1;" ... OpDecorate %7 LinkageAttributes "add" Export ... %7 = OpFunction %2 None %4 ...

I'm not sure how easy this output is to fix for the tests (or the ROI for such a change).

Adds the --no-link option which outputs the compiled shader binaries without linking them. This is a first step towards allowing users to create SPIR-v binary, non-executable libraries. When using the --no-link option, all functions are decorated with the Export linkage attribute.

arcady-lunarg

LGTM, thanks.

ncesario-lunarg force-pushed the compile-only branch from 288f8e1 to 7e56229 Compare September 6, 2023 00:34

ncesario-lunarg changed the title ~~RFC: Add --no-link option and [[export]] attribute~~ Add --no-link option to only compile (not link) individual shaders and export all functions Sep 6, 2023

ncesario-lunarg marked this pull request as ready for review September 6, 2023 15:17

ncesario-lunarg requested review from jeremy-lunarg and arcady-lunarg September 6, 2023 15:17

arcady-lunarg requested changes Sep 11, 2023

View reviewed changes

ncesario-lunarg force-pushed the compile-only branch from 7e56229 to d3806f2 Compare September 18, 2023 19:32

arcady-lunarg approved these changes Sep 18, 2023

View reviewed changes

arcady-lunarg merged commit 4c57db1 into KhronosGroup:main Sep 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --no-link option to only compile (not link) individual shaders and export all functions #3319

Add --no-link option to only compile (not link) individual shaders and export all functions #3319

ncesario-lunarg commented Aug 24, 2023 •

edited

Loading

ncesario-lunarg commented Sep 6, 2023

jeremyg-lunarg commented Sep 6, 2023

jeremyg-lunarg commented Sep 6, 2023

arcady-lunarg left a comment

arcady-lunarg Sep 6, 2023

ncesario-lunarg Sep 13, 2023

arcady-lunarg Sep 18, 2023

ncesario-lunarg Sep 18, 2023

arcady-lunarg Sep 11, 2023

ncesario-lunarg Sep 13, 2023

ncesario-lunarg Sep 13, 2023 •

edited

Loading

arcady-lunarg Sep 18, 2023

arcady-lunarg Sep 11, 2023

ncesario-lunarg Sep 13, 2023

arcady-lunarg Sep 11, 2023

ncesario-lunarg Sep 13, 2023

arcady-lunarg left a comment

Add --no-link option to only compile (not link) individual shaders and export all functions #3319

Add --no-link option to only compile (not link) individual shaders and export all functions #3319

Conversation

ncesario-lunarg commented Aug 24, 2023 • edited Loading

ncesario-lunarg commented Sep 6, 2023

jeremyg-lunarg commented Sep 6, 2023

jeremyg-lunarg commented Sep 6, 2023

arcady-lunarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncesario-lunarg Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcady-lunarg left a comment

Choose a reason for hiding this comment

ncesario-lunarg commented Aug 24, 2023 •

edited

Loading

ncesario-lunarg Sep 13, 2023 •

edited

Loading