Skip to content

Conversation

@NikitaRudenkoIntel
Copy link
Contributor

No description provided.

Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

Please add tests

case spv::ExecutionModeRoundingModeRTZ:{
unsigned TargetWidth;
N.get(TargetWidth);
BF->addExecutionMode(BM->add(new SPIRVExecutionMode(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see corresponding changes for the SPIRVReader, which means that this execution mode is going to be lost during reverse translation: are there plans to add SPIR-V -> LLVM IR support for this extension?

Copy link
Contributor Author

@NikitaRudenkoIntel NikitaRudenkoIntel May 19, 2020

Choose a reason for hiding this comment

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

The idea behind this commit is to introduce infrastructure, without any Reader/Writer part.
This particular change is also a part of infrastructure - before this piece of code will work, somebody has to add those Exec Modes to PreprocessMetadata.cpp
Until that, reader or writer will not even touch these exec modes: they just do not know from which structure in LLVM IR translate to ExecutionModeRoundingModeRTE, neither what to do with this mode in reader part.

@NikitaRudenkoIntel
Copy link
Contributor Author

The idea behind this patch is to separate introduction of SPV_KHR_float_controls extension from the particular implementation.
Thus, there is not much to test here: this commit basically extends existing enums, without providing any reader/writer parts.
On the other hand, it is the implementation, which will convert from LLVM IR to SPIRV and vice versa should (and will) introduce the testing

@AlexeySachkov
Copy link
Contributor

Thus, there is not much to test here: this commit basically extends existing enums, without providing any reader/writer parts.

But you have changed the writer part: it is now actually able to insert new decorations into SPIR-V file

@NikitaRudenkoIntel
Copy link
Contributor Author

I added the test, and moved commit with capabilities check here

Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me except the test: right now it doesn't produce valid SPIR-V and it would be good to fix.

@svenvh, @AlexeySotkin, any feedback?

@AlexeySachkov AlexeySachkov requested a review from svenvh May 25, 2020 18:46
Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@svenvh svenvh left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexeySachkov AlexeySachkov merged commit 3bc9ff0 into KhronosGroup:master May 27, 2020
AlexeySotkin pushed a commit that referenced this pull request Jun 26, 2020
Added support for `SPV_KHR_float_controls` extension to SPIR-V generator part
Fixed not emitting required extensions for capabilities

Change-Id: If71942c556fd0f315b3e6181bd1bc4e7552403e5
AlexeySotkin pushed a commit that referenced this pull request Jul 8, 2020
)

Added support for `SPV_KHR_float_controls` extension to SPIR-V generator part
Fixed not emitting required extensions for capabilities
AlexeySotkin pushed a commit that referenced this pull request Jul 17, 2020
)

Added support for `SPV_KHR_float_controls` extension to SPIR-V generator part
Fixed not emitting required extensions for capabilities
AlexeySotkin pushed a commit that referenced this pull request Jul 17, 2020
)

Added support for `SPV_KHR_float_controls` extension to SPIR-V generator part
Fixed not emitting required extensions for capabilities
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