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.

Sign up for GitHub

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

Jump to bottom

Integration of reporting api with translator parser #2457

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Integration of reporting api with translator parser #2457

wants to merge 1 commit into from

Conversation

bwlodarcz
Copy link
Contributor

From it's inception the reporting api had separate parsing pipeline - different from the rest of translator.
This fix integrate reporting api with the main translator parsing routines.
In addition to that it also fixes a problem of separate validation rules of reporting api.
Now, --spirv-print-report also obey command line validation rules (required extensions) like other commands.

asudarsa
asudarsa reviewed Mar 24, 2024
View reviewed changes
include/LLVMSPIRVOpts.h
@@ -285,6 +289,8 @@ class TranslatorOpts {
bool PreserveAuxData = false;

BuiltinFormat SPIRVBuiltinFormat = BuiltinFormat::Function;

bool IsReport = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a small comment about this field. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field only purpose is to go around iostream interface to make branch in parser routine and it won't last. Should be removed during removal of iostream implementation.

asudarsa
asudarsa reviewed Mar 24, 2024
View reviewed changes
lib/SPIRV/libSPIRV/SPIRVModule.cpp
@@ -2399,7 +2411,10 @@ std::istream &operator>>(std::istream &I, SPIRVModule &M) {
return MI.parseSPT(I);
}
#endif
return MI.parseSPIRV(I);
if (!MI.TranslationOpts.isReport()) {
return MI.parseSPIRV<false>(I);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not pass the isReport as an argument? Templated code might result in unneeded code bloating here.

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, at this stage no. We don't want to introduce unneeded branch in parser hot path for both report and binary cases. User using binary translation shouldn't pay price for Reporting API. This could be avoided if the implementation wouldn't be in iostreams. That makes efficient implementation of early stop very hard.
In this case although you have right that the code size will increase due to two implementations of the same function but prom performance PoV shouldn't make any difference. During first enter icache would be cold anyways and then it's circulating in the same hot routines. There won't be a situation when you have both <true> and <false> cases loaded to icache in the same time so really problem is negligible. What's more this template can be removed during removal of iostreams as main parsing system.

asudarsa
asudarsa reviewed Mar 24, 2024
View reviewed changes
test/spirv_report.spt
@@ -1,5 +1,5 @@
; RUN: llvm-spirv %s -to-binary -o %t.spv
; RUN: llvm-spirv --spirv-print-report %t.spv | FileCheck %s --check-prefix=CHECK-DAG
Copy link
Contributor

Choose a reason for hiding this comment

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

What will be reported if an extension is missing in spirv-ext? Can we add a CHECK for that?

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly the same like in other tests which covers situation with missing extensions. We already have such tests. For more detailed response please look on the answer below.

asudarsa
asudarsa reviewed Mar 24, 2024
View reviewed changes
Copy link
Contributor

@asudarsa asudarsa 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 overall. This is a useful refactoring. Thanks.
Few minor comments and clarifications requested.

@bwlodarcz
Integration of reporting api to parser
4c3e699 
From it's inception the reporting api had separate
parsing pipeline - different from the rest of translator.
This fix integrate reporting api with the
main parsing routines with translation.
In addition to that it also fixes a problem of separated validation
rules when it comes to reporting api. Now --report-spirv must also
 obey command line validation rules (required extensions)
like other commands.
@MrSidims MrSidims requested a review from asudarsa April 3, 2024 10:30
MrSidims
MrSidims reviewed Apr 3, 2024
View reviewed changes
lib/SPIRV/libSPIRV/SPIRVModule.cpp
@@ -2387,6 +2394,11 @@ std::istream &SPIRVModuleImpl::parseSPIRV(std::istream &I) {
SPIRVDBG(spvdbgs() << "getWordCountAndOpCode EOF 0 0\n");
break;
}
if constexpr (IS_REPORT) {
if (OpCode == OpMemoryModel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That does look fishy, why do we need to skip OpMemoryModel? There should be no more then 2 OpMemoryModel in the model.

test/negative/spirv_report_bad_input.spt
; RUN: echo "0" > %t_corrupted.spv && cat %t.spv >> %t_corrupted.spv
; RUN: not llvm-spirv --spirv-print-report %t_corrupted.spv 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR
;
; CHECK-ERROR: Invalid SPIR-V binary: "InvalidMagicNumber: Invalid Magic Number."
Copy link
Contributor

Choose a reason for hiding this comment

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

tbh I don't see this very error to be tested elsewhere

include/LLVMSPIRVLib.h
@@ -107,33 +107,34 @@ std::unique_ptr<SPIRVModule> readSpirvModule(std::istream &IS,
std::string &ErrMsg);

struct SPIRVModuleReport {
SPIRV::VersionNumber Version;
uint32_t Version;
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably shouldn't change a type for the Version

@asudarsa
Copy link
Contributor

asudarsa commented Apr 4, 2024

I am good with this PR. Think we should get this merged soon so that Bertrand can continue to work on improving it further. Thanks.

asudarsa
asudarsa approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for working on this Bertrand

@asudarsa asudarsa closed this Apr 4, 2024
@asudarsa
Copy link
Contributor

asudarsa commented Apr 4, 2024

close/open to rerun tests.

@asudarsa asudarsa reopened this Apr 4, 2024
@asudarsa
Copy link
Contributor

@bwlodarcz Can you please take a look at @MrSidims comments when convenient? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrSidims MrSidims MrSidims left review comments

@asudarsa asudarsa asudarsa approved these changes

@svenvh svenvh Awaiting requested review from svenvh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bwlodarcz @asudarsa @MrSidims