Skip to content

Reflect ExtInstSet generation steps of SPIRV-Tools in Android.mk.#190

Merged
antiagainst merged 1 commit into
google:masterfrom
antiagainst:spirv-tools-build
Apr 4, 2016
Merged

Reflect ExtInstSet generation steps of SPIRV-Tools in Android.mk.#190
antiagainst merged 1 commit into
google:masterfrom
antiagainst:spirv-tools-build

Conversation

@antiagainst
Copy link
Copy Markdown
Contributor

Counterpart for KhronosGroup/SPIRV-Tools#174.

Comment thread third_party/Android.mk
--extinst-glsl-grammar=$(SPVTOOLS_LOCAL_PATH)/source/extinst.glsl.std.450.grammar.json \
--extinst-opencl-grammar=$(SPVTOOLS_LOCAL_PATH)/source/extinst.opencl.std.grammar.json \
--core-insts-output=$(1)/core.insts.inc \
--glsl-insts-output=$(1)/glsl.std.450.insts.inc \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this script be run with a selection of arguments?
i.e. what happens if I were to omit --spirv-core-grammar="".

I ask because this would look a lot better as a set of 3(or 4) rules, 1 per target, rather than one large complicated one. It could also be factored into a single function so you could generate a new rule something like:

$(eval $(call gen_spvtools_grammar_tables, $(SPVTOOLS_OUT_PATH), core-insts-output, core.insts.inc)
$(eval $(call gen_spvtools_grammar_tables, $(SPVTOOLS_OUT_PATH), glsl-insts-output, glsl.std.450.insts.inc)

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.

I'm not sure that's the correct way to go because it means to duplicate the Capability enum in the glsl.std.450 & opencl.std grammar, too. That causes synchronization problem. Besides, glsl.std.450 & 'opencl.std` ExtInstSet depend on the core grammar naturally, so I'd prefer that consumers of these two ExtInstSets should refer to the core grammar.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's fair, I don't have a strong opinion, it just seems a bit unfortunate to have a single makefile rule that generates 4 different outputs. (And it's fairly conceivable to see it grow in the future)

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.

;) Sometimes (I mean, here) I feel strong pulse to insert tabs/spaces.

@AWoloszyn
Copy link
Copy Markdown
Contributor

+2 from me.

@dneto0
Copy link
Copy Markdown
Collaborator

dneto0 commented Apr 4, 2016

+2 (redundant), depending on KhronosGroup/SPIRV-Tools#174 going in

@antiagainst antiagainst merged commit 533ba9b into google:master Apr 4, 2016
@antiagainst antiagainst deleted the spirv-tools-build branch April 4, 2016 21:14
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