This is an archive of the discontinued LLVM Phabricator instance.

[SPIR-V 2/6] Add SPIRV{InstrFormats,InstrInfo,RegisterInfo,RegisterBanks...}.td
ClosedPublic

Authored by mpaszkowski on Dec 14 2021, 11:43 PM.

Details

Summary

The second patch in the effort of upstreaming the SPIR-V backend to the trunk. The patch adds initial TableGen sources: SPIRVInstrFormats.td, SPIRVInstrInfo.td, SPIRVRegisterBanks.td, SPIRVRegisterInfo.td, SPIRVEnums.td, and the main SPIRV.td. Please note that the patch will need to be amended/merged after D115009: [SPIRV 1/6] Add stub for SPIRV backend gets accepted and committed.

Diff Detail

Event Timeline

mpaszkowski requested review of this revision.Dec 14 2021, 11:43 PM
mpaszkowski created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2021, 11:43 PM

Can't see anything wrong with it but I don't know the SPIRV ISA, so...

llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
68

I'm guessing genV also defines SF?Cond and SI?Cond? Ie. the VS and VI are in addition to those.

Nit: The name of this patch is still "[2/n]"

svenvh added a subscriber: svenvh.Jan 11 2022, 4:15 AM
svenvh added inline comments.
llvm/lib/Target/SPIRV/SPIRV.td
44

Several files in this patch are missing their newline at EOF.

llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
89

The numbering doesn't correspond to the latest unified spec [1]. It is probably impractical to keep those numbers in sync as the spec keeps evolving. But perhaps we can provide a spec version number to avoid confusion?

[1] https://www.khronos.org/registry/SPIR-V/specs/unified1/SPIRV.html#_miscellaneous_instructions

152

Remove 1 newline here.

241

This can probably be removed?

493–495

This can probably be removed?

595

Please indent.

692–693

This patch doesn't add any instructions for these, so we could just as well remove these comments? Or alternatively, if we plan to support them, mark as TODO.

743

Not sure if this comment is relevant?

zuban32 added inline comments.Jan 11 2022, 2:04 PM
llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
68

Not sure I understand you comment. This multiclass may be used to generated any of scalar/vector integer/float versions of ternary instructions with either scalar or vectors first operand.

Now as I'm looking into it it seems quite redundant and irrelevant. It was introduced when we planned to move all of the selection stuff to tblgen from plain C++, but now it seems unrealistic so we may select OpSelect with C++ and remove this whole mclass.

mpaszkowski retitled this revision from [SPIR-V 2/n] Add SPIRV{InstrFormats,InstrInfo,RegisterInfo,RegisterBanks...}.td to [SPIR-V 2/6] Add SPIRV{InstrFormats,InstrInfo,RegisterInfo,RegisterBanks...}.td.Jan 12 2022, 1:02 AM
rengolin added inline comments.Jan 12 2022, 1:17 AM
llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
68

What I meant is that, in this definition, genV would have all 8 SF?Conv, SI?Conv, VS?Conv and VI?Conv, while not-genV would only have the initial 4.

I just wanted to make sure that was the intent, rather than genV only having the latter 4.

mpaszkowski marked 11 inline comments as done.Jan 27 2022, 3:07 AM

Resolved the comments and suggestions with the newly uploaded patch

rengolin accepted this revision.Feb 2 2022, 3:38 AM

All comments seem to have been addressed and the patch itself is very mechanical in nature, so LGTM.

This revision is now accepted and ready to land.Feb 2 2022, 3:38 AM
MaskRay accepted this revision.Mar 11 2022, 7:44 PM
MaskRay added inline comments.
llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
17
28
30
llvm/lib/Target/SPIRV/SPIRVRegisterInfo.td
40

No newline at end of file

Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2022, 7:44 PM
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.