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

[iliya-diyachkov]

Summary

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

[iliya-diyachkov]

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.

[rengolin]

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

[rengolin]

@MaskRay last round?

[iliya-diyachkov]

Rebase.

[MaskRay]

llvm/lib/Target/SPIRV/SPIRVRegisterBankInfo.cpp

16

This needs rebase after D119876

[iliya-diyachkov]

RegisterBank.h header is fixed.

[arsenm]

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]

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

[iliya-diyachkov]

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]

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

[iliya-diyachkov]

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.

[iliya-diyachkov]

The latest issues are fixed.

[mpaszkowski]

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

[MaskRay]

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

[iliya-diyachkov]

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]

All the issues found by Fangrui have been fixed.

[arsenm]

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp

90

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

[arsenm]

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

[iliya-diyachkov]

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.

[iliya-diyachkov]

The issues found by Matt have been resolved.

[MaskRay]

llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp

24

Then seems a good use of TODO or FIXME

[iliya-diyachkov]

llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp

24

Thanks, I will add.

[iliya-diyachkov]

TODOs in SPIRVISelLowering.cpp were added.

[iliya-diyachkov]

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

[rengolin]

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]

LGTM

[MaskRay]

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]

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.

[iliya-diyachkov]

@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.

[iliya-diyachkov]

The issues noted by Fangrui have been fixed.

[iliya-diyachkov]

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.