Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HLSL: implement numthreads for compute shaders #572

Merged
merged 1 commit into from Nov 1, 2016
Merged

HLSL: implement numthreads for compute shaders #572

merged 1 commit into from Nov 1, 2016

Conversation

ghost
Copy link

@ghost ghost commented Oct 28, 2016

HLSL: implement numthreads for compute shaders

This PR adds handling of the numthreads attribute for compute shaders, as well as a general
infrastructure for returning attribute values from acceptAttributes, which may be needed in other
cases, e.g, unroll(x), or merely to know if some attribute without params was given.

A map of enum values from TAttributeType to TIntermAggregate nodes is built and returned. It
can be queried with operator[] on the map. In the future there may be a need to also handle
strings (e.g, for patchconstantfunc), and those can be easily added into the class if needed.

New test is in hlsl.numthreads.comp.


if ((attr = attributes.find("numthreads")) != attributes.end()) {
// TODO: handle multiple entry points. TIntermediate presently only tracks one set of thread counts.
if (parseContext.parsingEntryPoint()) {
Copy link
Member

Choose a reason for hiding this comment

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

I considered exposing inEntryPoint before, for knowing if parameters were for functions or entry point, but the grammar doesn't know when that bit gets set, and it is not set until the entry point is partially parsed. That's how remapEntryPointIO() came about.

Given that the grammar can't depend on knowing whether parsingEntryPoint() is valid or not, is there a cleaner way, which keeps policy more cleanly in the parse helper?

E.g., acceptFunctionDefinition in the parse helper is where the decision is made about how to pass on numthreads?

namespace glslang {
class TIntermAggregate;

typedef std::unordered_map<TString, TIntermAggregate*> TAttributeMap;
Copy link
Member

Choose a reason for hiding this comment

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

Is string representing specific things like numthreads and flatten? What about the design alternative of using an enum { numthreads, flatten, ... max } and an array[max], so that string manipulation dies at parsing? Or does that break down?

@ghost
Copy link
Author

ghost commented Oct 30, 2016

Thanks for comments - repushed with these changes:

  1. No more exposure of inEntryPoint: attribute handling is now done in HlslParseContext::handleFunctionDefinition. Grammar is insulated from this detail.
  2. There's now an enum for valid attributes, defined in hlslAttributes.h, and a helper function attributeFromName() to accept a string and find the matching enum. The two operator[] functions on the TAttributeMap now accept an enum value. This slightly simplifies the consumer code.
  3. I added some other attributes into the list of parsed ones.

if (numThreadliterals != nullptr && inEntryPoint) {
const TIntermSequence& sequence = numThreadliterals->getSequence();

// TODO: handle multiple entry points. TIntermediate presently only tracks one set of thread counts.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I follow.

TIntermediate only handles one entry point, and generally the parser/intermediate design is centered around that. If a SPIR-V module were in the future to have multiple entry points, it would be because multiple TIntermediate each made a SPIR-V module, and the modules got merged (most recent thinking of how to do that), rather than the whole stack learned to handle multiple entry points.

Shader entry functions are different semantics from a regular function at the source level, in the AST, and in SPIR-V. So, I think the best way to handle, say, source with real multiple entry points is to compile it multiple times, once per entry point, and get correct SPIR-V modules for each, and then use a SPIR-V merger.

Copy link
Author

Choose a reason for hiding this comment

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

I see; that was down to a misunderstanding on my part then. Will fix comment.

@johnkslang
Copy link
Member

I think the code is fine. I only questioned a comment; curious if we're on the same page about it.

This PR adds handling of the numthreads attribute for compute shaders, as well as a general
infrastructure for returning attribute values from acceptAttributes, which may be needed in other
cases, e.g, unroll(x), or merely to know if some attribute without params was given.

A map of enum values from TAttributeType to TIntermAggregate nodes is built and returned.  It
can be queried with operator[] on the map.  In the future there may be a need to also handle
strings (e.g, for patchconstantfunc), and those can be easily added into the class if needed.

New test is in hlsl.numthreads.comp.
@ghost
Copy link
Author

ghost commented Oct 31, 2016

Removed WIP, re-pushed with a number of comment changes, updated commit message, and forward declaring the TAttributeMap class to avoid #including it from inside other headers.

@ghost ghost changed the title WIP: HLSL: numthreads HLSL: implement numthreads for compute shaders Oct 31, 2016
@johnkslang johnkslang merged commit 89df3c2 into KhronosGroup:master Nov 1, 2016
@ghost ghost mentioned this pull request Nov 16, 2016
51 tasks
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.

None yet

1 participant