Page MenuHomePhabricator

[SPIRV 6/6] Add the module analysis pass and the simplest tests
ClosedPublic

Authored by iliya-diyachkov on Dec 31 2021, 2:58 PM.

Details

Summary

This patch adds one SPIRV analysis pass and extends AsmPrinter. It is essential for minimum SPIR-V output. Also it adds several simplest tests to show that the target basically works.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
iliya-diyachkov retitled this revision from [SPIRV 6/6] Add one essential pass and the simplest tests to [SPIRV 6/6] Add the module analysis pass and the simplest tests.
iliya-diyachkov edited the summary of this revision. (Show Details)

The functionality of SPIRVGlobalTypesAndRegNum pass was moved to SPIRVModuleAnalysis pass and extended AsmPrinter.

The formatting issue is fixed.

I think all noticed issues are fixed in this patch, as well as in others in the series. The patches were rebased to the latest LLVM version (0712c575b90a7eb508bf43d15c38c1c0b0d69695, Fri Feb 18). Also the regular and special builds (suggested by Fengrui Song in the first patch) were passed without new fails in LIT tests.

@arsenm @MaskRay could you please review the updated patches again.

Move MS static array inside struct ModuleAnalysisInfo.

Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2022, 4:10 PM

@arsenm @MaskRay could you please review the patches again?

As I wrote earlier, all issues were fixed in all patches of the series. The day before yesterday I rebased all patches to the latest LLVM version (17a68065c378, Sun Mar 6) and checked the regular and special builds (suggested in the first patch).

Fixed typos in comments.

Update after the last changes in the previous patches.

Update after the last changes in the 3th and 4th patches.

iliya-diyachkov updated this revision to Diff 415659.EditedMar 15 2022, 6:40 PM

Update after the last change in the 5th patch.

The format issue is fixed.

Gentle ping for final review and acceptance :)

Hi @arsenm, could you please take a look at the 4th, 5th, and 6th patch and let us know whether your suggestions were resolved? We would like to kindly ask for "LGTM".

Gentle ping.

@arsenm @MaskRay could you please continue or finish the review of the 4th, 5th and 6th patches? As I wrote earlier, all the issues were fixed in all patches of the series.

I updated LLVM to today's revision 73777b4c35a3 and retested all recommended builds. All of them are passed.

The patch has been updated after the changes in 4th and 5th ones. Also, "unsigned int"s were replaced with "unsigned"s, and missed dots were inserted into comments, as Fangrui recommended in the 4th patch.

Update after the change in the 3rd patch.

arsenm added inline comments.Mar 28 2022, 1:43 PM
llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp
43

Might as well use SPIRVSubtarget?

47

getSubtarget<SPIRVSubtarget>() instead of static_cast

118

Can't have global constructors

136

Typo correspnding

137

static?

161

If this isn't checked by the verifier (which would be preferable), should be report_fatal_error

llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
162

I don't think this can fail at this point? If not, should early continue or assert and reduce indentation

201

Ditto

llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp
43

You probably mean removing STI and using OutContext.getSubtargetInfo().

47

As I see we need to have either MachineFunction or Function to call getSubtarget<SPIRVSubtarget>().
However in SPIRVAsmPrinter ST is required even if there are no functions in the module. I don't see any other way than to use static_cast as some other targets do (e.g. in NVPTXAsmPrinter::emitStartOfAsmFile or in AVRAsmPrinter::emitStartOfAsmFile). If you know, please advise.

118

I'll move BBNumToRegMap into struct ModuleAnalysisInfo.

137

getOrCreateMBBRegister() was not static since it was called in SPIRVMCInstLower.cpp (outside of this module). I'll make getOrCreateMBBRegister() a member of struct ModuleAnalysisInfo as well as BBNumToRegMap.

161

I believe that it is not checked. I'll add the appropriate TODO and change llvm_unreachable to report_fatal_error.

llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
162

This can fail if F is a declaration. I'll add early continue.

iliya-diyachkov marked 8 inline comments as done.Mar 28 2022, 6:33 PM

The patch was updated after the last changes in the 5th one. Also ModuleSectionType and ModuleAnalysisInfo were placed in the SPIRV namespace.

The changes in SPIRVUtils from the 6th patch were moved to the 5th patch. The last case of "unsigned int" was replaced with "unsigned".

@arsenm, @MaskRay, I think all the issues have been fixed. Could you please take the last look at the patches?

This one still has some unactioned comments...

This one still has some unactioned comments...

@rengolin, do you mean that some issues pointed out by Matt have not been fixed? I thought everything was done, except for the suggestion to use getSubtarget<SPIRVSubtarget>() instead of static_cast in SPIRVAsmPrinter constructor. I explained that it was not clear how to efficiently apply this suggestion to our case.

Apologies, it seems I was looking into a very old version of the review!

This looks good to me. @arsenm did that answer your comment?

arsenm added inline comments.Apr 7 2022, 6:34 AM
llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp
47

Oh, this points out a deeper issue that you're using the global subtarget. Ideally you're always querying the subtarget from the context function

arsenm added a comment.Apr 7 2022, 6:35 AM

Apologies, it seems I was looking into a very old version of the review!

This looks good to me. @arsenm did that answer your comment?

Yes, but it points to another issue (one that SPIRV specifically probably doesn't care about)

iliya-diyachkov added inline comments.Apr 7 2022, 8:01 AM
llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp
47

I understand this, but in some cases we have a module without functions, and we still need to use Subtarget.

We could get Subtarget using static_cast when initializing AsmPrinter. Then if there are MachineFunctions in the module, we will get Subtarget using MF.getSubtarget<SPIRVSubtarget>() in emitFunctionHeader() for each MachineFunction.
Does this approach make sense?

arsenm added inline comments.Apr 7 2022, 8:02 AM
llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp
47

Yes, each function should query the subtarget

iliya-diyachkov added inline comments.Apr 7 2022, 8:05 AM
llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp
47

Ok, I'll implement it later today.

iliya-diyachkov updated this revision to Diff 421347.EditedApr 7 2022, 3:02 PM

Query ST for each function (in SPIRVAsmPrinter).

iliya-diyachkov marked 2 inline comments as done.Apr 7 2022, 3:13 PM

Updated after the last changes in the 4th and 5th patches.

Ping! We kindly ask for "LGTM" :)

Gentle ping. @arsenm please take a look at the final version of this patch.

arsenm accepted this revision.Apr 18 2022, 5:36 PM

LGTM with nit

llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
210–216

Could use another continue and indentation reduction

This revision is now accepted and ready to land.Apr 18 2022, 5:36 PM

The indentation issue has been fixed.

iliya-diyachkov marked an inline comment as done.Apr 19 2022, 2:34 AM

Thank you for finishing the review of this series, Matt!

This revision was landed with ongoing or failed builds.Apr 19 2022, 4:28 PM
This revision was automatically updated to reflect the committed changes.