This is an archive of the discontinued LLVM Phabricator instance.

[SPIRV] add PrepareFunctions pass and update other passes
ClosedPublic

Authored by iliya-diyachkov on Jul 13 2022, 9:28 PM.

Details

Summary

The patch adds PrepareFunctions pass, which modifies function signatures containing aggregate arguments and/or return value before IR translation. Information about the original signatures is stored in metadata. It is used during call lowering to restore correct SPIRV types of function arguments and return values. This pass also substitutes some llvm intrinsic calls to function calls, generating required functions in the module as the SPIRV translator does.

Also the patch includes the changes in other modules, enabling many SPIRV features that were not very functional without DT patch by Aleksandr and were omitted earlier. As a result, many new LIT tests began to pass, 15 of them are also included into the patch, others will be added later.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 9:28 PM
iliya-diyachkov requested review of this revision.Jul 13 2022, 9:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 9:28 PM

Fix one clang-format issue in SPIRVPrepareFunctions.cpp.

iliya-diyachkov retitled this revision from [SPIRV] add PrepareFunctions pass and update other passed to [SPIRV] add PrepareFunctions pass and update other passes.

Add missed changes, correct some added tests.

mpaszkowski accepted this revision.Jul 21 2022, 6:29 AM

Thank you for the patch and correcting the issues @iliya-diyachkov! LGTM!

This revision is now accepted and ready to land.Jul 21 2022, 6:29 AM

To me it seems as an unnecessarily huge change. Can't it be split, at least keep PrepareFunctions and other minor changes separate?

To me it seems as an unnecessarily huge change. Can't it be split, at least keep PrepareFunctions and other minor changes separate?

Well, it looks big, but comparable to the patches from the initial series (and even smaller then some of them). By the way, the changes in most files are quite minor. Only 5 files are significantly changed (SPIRVAsmPrinter.cpp, SPIRVCallLowering.cpp, SPIRVGlobalRegistry.cpp, SPIRVInstructionSelector.cpp, SPIRVModuleAnalysis.cpp), and just one new pass is added.

It's well tested and provides a good improvement in the stability and functionality of the SPIRV backend, so I wanted to introduce it before the 15th release. If the size is not a big issue and you don't mind, I will commit it as it is.

To me it seems as an unnecessarily huge change. Can't it be split, at least keep PrepareFunctions and other minor changes separate?

Well, it looks big, but comparable to the patches from the initial series (and even smaller then some of them). By the way, the changes in most files are quite minor. Only 5 files are significantly changed (SPIRVAsmPrinter.cpp, SPIRVCallLowering.cpp, SPIRVGlobalRegistry.cpp, SPIRVInstructionSelector.cpp, SPIRVModuleAnalysis.cpp), and just one new pass is added.

It's well tested and provides a good improvement in the stability and functionality of the SPIRV backend, so I wanted to introduce it before the 15th release. If the size is not a big issue and you don't mind, I will commit it as it is.

I won't insist this particular time, but its size just doesn't allow a proper review of the changes, so pls try to make changes better structured in the future. IMO the backend has enough code already to stop this 'Added feature X + many minor fixes' approach, we need to match the overall project's style of commits.

OK, thanks. I'll take that into account next time.

This revision was landed with ongoing or failed builds.Jul 21 2022, 5:19 PM
This revision was automatically updated to reflect the committed changes.