This is an archive of the discontinued LLVM Phabricator instance.

[SPIRV 4/6] Add target lowering, TargetMachine and AsmPrinter
ClosedPublic

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

Details

Summary

The patch contains target lowering for SPIRV. Also it implements TargetMachine and AsmPrinter.

Diff Detail

Event Timeline

iliya-diyachkov requested review of this revision.Dec 31 2021, 2:46 PM
Herald added a project: Restricted Project. · View Herald Transcript

I'm guessing that the missing bits (ex. frame lowering) will be implemented in following patches.

llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
45

Don't keep code commented out in patches. If you need this somewhere, keep it local and reapply later when it makes sense to do so.

Yes, some missing features will be added later. What about frame lowering, the SPIRV target is very special: it uses only virtual registers. It does not operate with stack frame explicitly and does not generate a function prologue/epilogue. So it does not actually utilize frame lowering and we do not plan to add more functionality there.

llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
45

Sorry, it will be removed.

Yes, some missing features will be added later. What about frame lowering, the SPIRV target is very special: it uses only virtual registers. It does not operate with stack frame explicitly and does not generate a function prologue/epilogue. So it does not actually utilize frame lowering and we do not plan to add more functionality there.

I suspected as much. So it would be nice to add a comment there saying these are not used because of the SPIRV lowering.

Ok, I'll add the comment to SPIRVFrameLowering.h.

A comment about frame lowering in SPIRV was added (SPIRVFrameLowering.h).
Unnecessary if0 was removed in SPIRVISelLowering.cpp.
Unwanted "unsing namespace" was removed in SPIRVInstrInfo.cpp.

MaskRay added inline comments.Jan 3 2022, 3:46 PM
llvm/lib/Target/SPIRV/SPIRVMCInstLower.h
19

https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments "Don’t duplicate function or class name at the beginning of the comment."

22

delete unneeded ctor

llvm/lib/Target/SPIRV/SPIRVRegisterBankInfo.h
35

delete unneeded ctor. ditto in many places

llvm/lib/Target/SPIRV/SPIRVRegisterInfo.cpp
31

Use CSR_NoRegs_SaveList instead

llvm/lib/Target/SPIRV/SPIRVRegisterInfo.h
25

delete unneeded blank line. ditto in many places

llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
64

The canonical form is /*TuneCPU=*/CPU accepted by both clang-format and clang-tidy.

88

remove used-once variable

llvm/lib/Target/SPIRV/SPIRVSubtarget.h
60

This should be fixed

107

delete unneeded function.

arsenm added inline comments.Jan 4 2022, 10:35 AM
llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
78

This doesn't look like a subtarget property, and also depending on the language seems conceptually problematic

83

Ditto

88

The SPIR version seems like it should be a target machine level version, not subtarget property

arsenm added inline comments.Jan 4 2022, 10:37 AM
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
37

No else after return

arsenm added inline comments.Jan 4 2022, 10:55 AM
llvm/lib/Target/SPIRV/SPIRVTargetMachine.h
46

The select implementation disagrees?

llvm/lib/Target/SPIRV/SPIRVRegisterInfo.cpp
31

It looks like this was borrowed from AMDGPU (see R600RegisterInfo::getCalleeSavedRegs in R600RegisterInfo.cpp).
SPIRV target does not use physical registers and does not define SPIRVCallingConv.td. Several targets (like NVPTX, WebAssembly) do not use *CallingConv.td and just have

static const MCPhysReg CalleeSavedRegs[] = { 0 }

Why wouldn't SPIRV use the same?

llvm/lib/Target/SPIRV/SPIRVSubtarget.h
60

Sorry, this comment doesn't apply to these pointers. They are standard GlobalISel related APIs. I'll correct the comment.

llvm/lib/Target/SPIRV/SPIRVTargetMachine.h
46

Yes, it's an error.

llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
88

@arsenm I guess you propose to make the SPIR-V version an analogue of Processor and represent the version's features as SubtargetFeatures. Then we fill each "Processor" (i.e. the SPIR-V version) with its features and the features become subtarget property.

It looks good, but as I see there is an alternative approach: NVPTX's subtarget contains SmVersion that is used to evaluate subtarget's features in methods of NVPTXSubtarget class. The SPIR-V implementation uses the similar manner defining TargetSPIRVVersion as a member of SPIRVSubtarget and evaluating subtarget's features using the version number. Not all the subtarget's methods (which use the version) are published in the patch series since they are connected to the parts that are not yet present in it.

Perhaps this is a reasonable argument for keeping TargetSPIRVVersion a member of Subtarget. Maybe we'd better change its name to shorter and less confusing e.g. "SPIRVVersion".

What about other version members (TargetOpenCLVersion, TargetVulkanVersion), we don't use them in this series so they may be currently omitted for now.

The issues found by Fangrui Song and Matt Arsenault were fixed.

iliya-diyachkov marked 15 inline comments as done.Jan 11 2022, 4:15 PM

The updated patch fixes builds with shared libs, which Fangrui Song proposed to check. This patch does not cause "x64 debian fail" (9 failed tests in libarcher :: races) since I see other builds that fail with the same issue.

Phab comments are out of place in Subtarget, so I'll let @arsenm approve this one if his comments were addressed.

Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2022, 3:31 PM
MaskRay added inline comments.Mar 11 2022, 7:36 PM
llvm/lib/Target/SPIRV/SPIRVRegisterBankInfo.cpp
16

This needs rebase after D119876

iliya-diyachkov updated this revision to Diff 414885.EditedMar 12 2022, 3:00 PM

RegisterBank.h header is fixed.

iliya-diyachkov marked an inline comment as done.Mar 12 2022, 3:02 PM
arsenm added inline comments.Mar 14 2022, 3:28 PM
llvm/lib/Target/SPIRV/SPIRVRegisterInfo.cpp
31

It's the same thing, not much difference. It's not worth defining SPIRCallingConv.td just for this

llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
36–37

Why is there a fallback to 8? Why not just switch or check if 64?

llvm/lib/Target/SPIRV/SPIRVTargetMachine.h
46

Should this be removed then?

arsenm added inline comments.Mar 14 2022, 3:56 PM
llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp
65

Braces

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
32

You can't guarantee this. A return of struct with multiple fields will hit it

57

This will happen for any struct arguments

90

Struct returns

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
32

In SPIR-V spec composite returns/formal args are allowed. In the case of composites we need to keep the original type info and to avoid lowering of composite regs to multiple scalar regs. To achieve this we introduce a pass (it's not included in this series) before IR translation which clones such functions with composite returns/args to functions with scalar returns/args and keeps composite type info in metadata. So at the moment of CallLowering invocation we always expect VRegs size=1.
After this we restore the type info and associate it with the function type. Finally we get functions with composite returns/args at output.
In the absence of this pass in the series we could temporary remove the asserts but I'm not sure if it's necessary change. Maybe it's better to add a comment about it?

57

Explained above.

90

Explained above.

llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
36–37

In fact, we have one more target ("logical") and this 8 relates to it. However it is not included in this series, so I'll remove this fallback.

llvm/lib/Target/SPIRV/SPIRVTargetMachine.h
46

You are right, I missed that it is the default behavior.

arsenm added inline comments.Mar 14 2022, 5:38 PM
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
32

It's still valid IR, so you should do something better than assert. Also needs some test coverage

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
32

We have LIT tests with composite returns/args, but I'm afraid it's rather difficult to use them at the stage of this patch series, because the required functionality is not yet included and all the tests will fail.
Taking into account your advice, I assume that adding "// TODO..." before these asserts may be an appropriate solution for now.

The latest issues are fixed.

iliya-diyachkov marked 8 inline comments as done.Mar 15 2022, 5:02 AM
RKSimon retitled this revision from [SPIRV 4/6] Add target lowerling, TargetMachine and AsmPrinter to [SPIRV 4/6] Add target lowering, TargetMachine and AsmPrinter.Mar 15 2022, 6:37 AM

Gentle ping! We would like to ask for a final "LGTM".

MaskRay added inline comments.Mar 27 2022, 10:58 AM
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
45

Just return 0.

Is this stub necessary?

56

s/unsigned int/unsigned/g

This applies to many files.

llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
16

delete

llvm/lib/Target/SPIRV/SPIRVMCInstLower.cpp
31

Delete print in the unreachable branch.

llvm/lib/Target/SPIRV/SPIRVMCInstLower.h
22

lower

llvm/lib/Target/SPIRV/SPIRVRegisterInfo.cpp
27

return BitVector(getNumRegs())

llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
111
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
45

This stub is filled in the 5th patch. Since this is confusing, I'll remove it here and add it in the 5th patch.

56

Ok, I'll fix it in all files.

llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
16

Sorry.

iliya-diyachkov marked 7 inline comments as done.Mar 27 2022, 7:29 PM

All the issues found by Fangrui have been fixed.

arsenm added inline comments.Mar 28 2022, 2:02 PM
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
90

It's still valid IR, so should do better than assert. Return false to fail cleanly

arsenm added inline comments.Mar 28 2022, 2:04 PM
llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
24

Do you need either of these hacks anymore? The support for odd sized vectors has improved in recent years

llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
24

It's still needed, otherwise CallLowering fails inside getVectorTypeBreakdown on some tests which passes/returns <3 x i1> vectors. Once it's supported by default code, we'll remove this hacks.

The issues found by Matt have been resolved.

iliya-diyachkov marked 2 inline comments as done.Mar 29 2022, 5:27 AM
MaskRay added inline comments.Mar 29 2022, 1:19 PM
llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
24

Then seems a good use of TODO or FIXME

llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
24

Thanks, I will add.

TODOs in SPIRVISelLowering.cpp were added.

iliya-diyachkov marked an inline comment as done.Mar 29 2022, 1:50 PM

@MaskRay, @arsenm, is this patch (after the last changes) fine with you?

Seems most (all?) comments were addressed, @arsenm is there anything critical that needs done before the merge of the first batch?

The initial reviews have a higher bar to cover, but unless it's something controversial, we could fix smaller things on following reviews?

arsenm accepted this revision.Apr 5 2022, 5:03 PM

LGTM

This revision is now accepted and ready to land.Apr 5 2022, 5:03 PM
MaskRay added inline comments.Apr 7 2022, 11:03 PM
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
53

There is a Clang -Wunused-variable warning.

llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
47

make_unique

50

RegBankInfo = std::make_unique<SPIRVRegisterBankInfo>();

MaskRay accepted this revision.Apr 7 2022, 11:26 PM

I was on a trip for one week.

llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp
156

Can you verify this is unreachable? If it's unimplemented, you may call report_fatal_error

176

ditto

llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
37

The convention is to add isSPIRV (see isX86) and use it.

llvm/lib/Target/SPIRV/SPIRVSubtarget.h
70

I'd omit blank lines among these getter functions. aarch64/riscv use the compact style.

llvm/lib/Target/SPIRV/SPIRVTargetObjectFile.h
21

Delete unneeded default ctor

32

The 3 functions need a comment. Using a non-data section seems weird to readers.

@MaskRay, thanks for your comments. I'll fix the issues later today.

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
53

Yes, but Arg is used in the next 5th patch so the warning disappears. I guess it's acceptable to leave this until the 5th patch application, isn't it?

llvm/lib/Target/SPIRV/SPIRVTargetObjectFile.h
32

Ok, I'll add a comment saying that all contents of SPIRV file are instructions, so we put everything in a text section.

The issues noted by Fangrui have been fixed.

iliya-diyachkov marked 9 inline comments as done.Apr 8 2022, 5:58 PM
iliya-diyachkov added inline comments.
llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp
156

At the current stage we are not going to support branch optimization. But I admit that this may be implemented in the future, so let's change it to report_fatal_error.

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.