Page MenuHomePhabricator

[SPIRV 1/6] Add stub for SPIRV backend
ClosedPublic

Authored by iliya-diyachkov on Dec 2 2021, 4:36 PM.

Details

Summary

This patch contains enough for lib/Target/SPIRV to compile: a basic SPIRVTargetMachine and SPIRVTargetInfo.

Triples for SPIRV were recently added (see D109144).

Diff Detail

Event Timeline

iliya-diyachkov created this revision.Dec 2 2021, 4:36 PM
iliya-diyachkov requested review of this revision.Dec 2 2021, 4:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2021, 4:36 PM
zuban32 added a subscriber: zuban32.Dec 2 2021, 4:58 PM

I don't see any recent discussion of this project on llvm-dev. Please make sure you send an email to llvm-dev stating your plan to land an experimental SPIRV target, and explains how it meets the requirements outlined at https://llvm.org/docs/DeveloperPolicy.html#adding-a-new-target . (I don't see any issue meeting those requirements, but let's make sure everyone is on the same page.)

Just to clarify, the high level design for the proposal has been sent to llvm-dev in March this year.

https://lists.llvm.org/pipermail/llvm-dev/2021-March/148905.html

But I agree, we should send some heads up announcement that we are starting to land patches and they are going to be an experimental backend.

Ilya has posted an official RFC announcing the upstream of SPIR-V backend as an experimental target:
https://lists.llvm.org/pipermail/llvm-dev/2021-December/154270.html

You could mark the other revisions as child revisions with Edit Related Revisions... at the top right.

Hi, I only saw that now.

In addition to adding the rest of the reviews as a child to this one, can you also please number them accordingly?

Please use 1/10, 2/10, etc. instead of 1/n, 2/n. It makes no sense to have a '/n'.

Also, make sure that the last patch allows the target to do something substantial (with tests), for example, parse/lower assembly instructions, lower a simple function with return, etc.

Once the first series is complete, you don't need to number the patches any more. That's the whole point of the '/n', to separate the before, where nothing works, with the after, where things progressively work better.

Finally, just a reminder that you should not merge any patch in the initial series until *all* patches in the series have been approved. This is crucial to avoid reverting patches on a half-implemented back-end.

Thanks!
Renato

kpet added a subscriber: kpet.Dec 16 2021, 9:51 AM

Hi Renato. Great thanks for your clear explanation! We'll rename the patches and provide the complete series for review.

iliya-diyachkov retitled this revision from [SPIRV 1/n] Add stub for SPIRV backend to [SPIRV 1/6] Add stub for SPIRV backend.

It's a small update with capitalized names of variables.

Hi, this looks good as the first patch. Will keep looking for the others.

Confusing comment in SPIRVTargetMachine.cpp was removed.

MaskRay added a comment.EditedJan 3 2022, 3:59 PM

Other than the regular -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=on builds, it's good to ensure a few others work:

  • -DBUILD_SHARED_LIBS=on ensure CMake build dependency is correct. The mode uses -Wl,-z,defs to link shared objects. It has some dependency checking effects: https://maskray.me/blog/2021-06-13-dependency-related-linker-options#z-defs)
  • -DLLVM_LINK_LLVM_DYLIB=on -DCLANG_LINK_CLANG_DYLIB=on: somewhat covered by -DBUILD_SHARED_LIBS=on, but not bad to ensure it works as many Linux distros have shifted to this mode
  • -DLLVM_ENABLE_EXPENSIVE_CHECKS=on -DLLVM_ENABLE_ABI_BREAKING_CHECKS=on
  • omitting -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=SPIRV works
arsenm added inline comments.Jan 4 2022, 8:56 AM
llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
40

Don’t need .hasValue()

Ok, after fixing the issues in all patches I'll check these builds.

SPIRVTargetMachine.cpp was fixed.

iliya-diyachkov marked an inline comment as done.Jan 9 2022, 1:49 PM

Thank you to everyone for all the suggestions and feedback. In the recent weeks we have resolved all the comments and submitted all the necessary fixes to the initial six submitted patches:

  1. SPIRV 1/6 Add stub for SPIRV backend
  2. SPIRV 2/6 Add SPIRV {InstrFormats,InstrInfo,RegisterInfo,RegisterBanks...}.td
  3. SPIRV 3/6 Add MC layer, object file support and InstPrinter
  4. SPIRV 4/6 Add target lowerling, TargetMachine and AsmPrinter
  5. SPIRV 5/6 Add LegalizerInfo, InstructionSelector and utilities
  6. SPIRV 6/6 Add 2 essential passes and the simplest tests

Right now all the patches are passing buildbot builds and checks (with the exception of a single clang-format comment on the common across all targets Triple.cpp file in the third patch). We would love to ask you for the final review and LGTM of the backend stub and the six patches above as a whole, so we will be able to push the changes to the trunk soon.

I also want to express my gratitude to the reviewers.

In addition to the regular builds, I also checked the special ones that Fengrui Song listed earlier (all with and without -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=SPIRV). I fixed a few issues in the 3th-6th patches, so now all additional builds (as well as the regular ones) are build-able, and built llcs successfully pass LIT testing (including added SPIRV's LIT tests).

We look forward to the final feedback or further comments on the code.

In addition to the regular builds, I also checked the special ones that Fengrui Song listed earlier (all with and without -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=SPIRV). I fixed a few issues in the 3th-6th patches, so now all additional builds (as well as the regular ones) are build-able, and built llcs successfully pass LIT testing (including added SPIRV's LIT tests).

Great! That's a good shape to have the initial series. Thanks for the hard work!

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

This looks good to me as the first patch.

If others still have comments, please don't shy on posting them. If not, feel free to approve, too.

This revision is now accepted and ready to land.Feb 2 2022, 3:37 AM

@MaskRay last round?

Thanks for tagging me. Will look:)

Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2022, 2:20 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
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.