This is an archive of the discontinued LLVM Phabricator instance.

[SPIRV 5/6] Add LegalizerInfo, InstructionSelector and utilities
ClosedPublic

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

Details

Summary

The patch adds SPIRVLegalizerInfo, SPIRVInstructionSelector and SPIRV-specific utilities.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
arsenm added inline comments.Jan 4 2022, 11:05 AM
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
84–85

This should probably be an assert, not report_fatal_error

215–216

should be assert?

277

No const reference

llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
10–13

Why do you need to do this? What are you losing in gMIR that matters?

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
205–208

You shouldn't have to construct a fresh new MIRBuilder for every selected instruction

519

I agree there are too many autos

593–610

I think you are just repeating the generic expansion for this. It would be better to just use the default lower action for G_ATOMIC_CMPXCHG_WITH_SUCCESS

596

LLT is shorter and clearer than auto

661

return after else

836–844

It looks like you're repeating work done in the legalizer?

1102

No else after return

1139

Should just use cast<> and remove the check, this should never fail. I believe we disallow null G_GLOBAL_VALUE sources

llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp
178

I thought SPIRV had a fixed set of register types?

203–204

This is illegal MIR, there's no point in checking it

205

Else after return

206

Typo ponter

218–219

You don't need to worry about the cases that are illegal MIR

220

Else after return

234

This has been not true for about 2 years

287

I'm not sure what's meant by an "Id Reg"

302

Are you intentionally discarding the pointer size and address space?

311

Generally code in the legalizer shouldn't bother setting regbanks or classes

333

You're doing a lot of work to convert to the SPIRV types here, which would be more appropriate to do during selection

349–356

it might be cleaner to push this cast to integer into the generic LegalizerHelper

362–366

You're assuming there's only a single use, and your legalize action should only touch the passed instruction

llvm/lib/Target/SPIRV/SPIRVUtils.cpp
133–134

How/why are you not using generic as addrspace 0?

161–170

I dislike this wrapper function. You should have all of these variables available in the context where you are constraining already, and the MF parameter with null check is potentially confusing

iliya-diyachkov added inline comments.Jan 5 2022, 4:19 AM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h
777

Ok, "assert((Op.getImm() >> 32) == 0..." will be added.

llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
10–13

SPIR-V annotates each instruction with type IDs. SPIR-V uses high-level types including composites (structs, arrays...) and some others. Most of them can be obtained from LLVM Types but they are not available in gMIR, so we need to save and maintain own type information.

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
519

The autos will be cleaned.

836–844

Yes, it looks like I need to fix this.

1139

Ok, I'll check this.

llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp
30

It will be replaced by std::set.

llvm/lib/Target/SPIRV/SPIRVUtils.cpp
133–134

One of the important features of this backend is compatibility with SPIRV-LLVM-Translator. We follow the address spaces agreement announced in https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/docs/SPIRVRepresentationInLLVM.rst#address-spaces .

zuban32 added inline comments.Jan 10 2022, 10:39 AM
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp
178

In SPIRV [almost] every value is an ID which is just an integer (usually 32- or 64-bit ), the type of the value with an ID is detemined by the instruction producing it which has 'ResultType' parameter. But we're trying not to enforce that rule on gMIR level so that before the selection all of the types were the ones IRTranslator emitted without anyhting target-specific interfering.

302

Well, yes and no. In fact LLTs do not matter after we generated all the necessary type-assigning pseudo instructions. But LLTs still matter in sence of gMIR being well-formed (passing verification) and not having any significant differences from the original IR (i.e. scalars instead of vectors, vectors of different type, etc) The addrspace of LLT doesn't matter for sure

333

We placed this code here because it's doing some sort of legalization (in old SPIR-V versions pointers can't be compared directly, ptrtoints are required), not sure if it's good to mix things up in the selector.

362–366

The single-use constraint is enforced by the design of our target, every non-void instruction have the only user - type assignment instruction, which in turn may have multiple users just as the original instruction before.

arsenm added inline comments.Jan 10 2022, 10:48 AM
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp
178

That's the whole point of the legalizer. There's some notion of fixed supported types implied by the other legalizer rules

362–366

This is not a property the target can decide, the incoming IR can have multiple uses. It's simply wrong to rely on it like this

zuban32 added inline comments.Jan 10 2022, 11:44 AM
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp
362–366

Why not if we have a target-specific LLVM IR pass making that assumption always true?

llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp
29

Ok, it will be applied to other expanded macros that also return literals.

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
132–142

I'm not sure that it is checked by verifier.
I'm sorry, this code fragment is not needed right now (since the series of patches does not include the pass that generates this metadata). I'll exclude it from this patch. We'll verify and correct the fragment by the next time.

172

Thanks, it will be fixed.

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
205–208

As I understand you suggest constructing MachineIRBuilder closer to its users, because not all instructions (for selection) are actually use it.

593–610

Yes, I'll fix it.

llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp
178

@arsenm yes, all register types are legal for phis. This rule will be substituted by legalFor(allPtrsScalarsAndVectors).

287

I suppose it means a register which is used in GET_ID pseudo instruction.

302

It's used in creation of new temporary pseudos which are involved in the instruction selection and then are removed. Perhaps it's a bad practice and should be avoided.

311

Probably it would be better to move this to selector.

333

@arsenm Yes, we'll try to move all this work to the selection stage.
@zuban32 I have verified that we don't need this code here (see Khronos repo).

349–356

Ok, I have created a function that generates a single cast.

362–366

Instructions processed here should always have a single use which is a pseudo instruction ASSIGN_TYPE, generated by SPIRV specific pass before the legalizer. We'll add a check that this condition is met. Also it looks like the modification of another instructions may be avoided.
And this fragment works together with another pass that is not presented in this series, so I would postpone it for now.

llvm/lib/Target/SPIRV/SPIRVUtils.cpp
161–170

Ok, we'll get rid of it.

zuban32 added inline comments.Jan 10 2022, 11:47 AM
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp
362–366

Oh, I see. It's just the pass responsible for making the input IR compliant with what SPIR-V specific GlobalISel stages expect is currently missing from these patches. Maybe we need to extend the series

llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp
362–366

As Renato suggested in the comment (see the first patch), we need a minimum set of patches that does something substantial. Then we'll add passes/features by stand alone patches extending the SPIR-V backend functionality and making more tests passed. I'm not sure that right now we have a reason to increase the series noticeably.

zuban32 added inline comments.Jan 10 2022, 12:14 PM
llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp
362–366

Yep, I understand that. The question is whether initial series should be kind of transitive closure or there might be some logical inconsistency due to some parts of the code not being upstreamed yet.

llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp
362–366

Of course the series should be consistent but I think it doesn't make sense for us to strive for transitive closure at this stage, because it is only the first step towards adding (to LLVM) a full-featured SPIR-V backend, which will be useful for end users. Approaching this goal in accordance with the logic of the LLVM project is the meaning of the current work.

Probably it's worth adding more functionality to SPIRVGlobalTypesAndRegNumPass.cpp so more tests from our test base can initially pass. But as Matt commented we should make changes in the way SPIRVGlobalTypesAndRegNumPass works, I'm going to try it.

iliya-diyachkov marked an inline comment as done.

The issues found by Renato Golin, Fangrui Song and Matt Arsenault were fixed. The macros in BaseInfo were expanded.

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

Too many comments for me to follow and check, I'll let @MaskRay and @arsenm approve this one.

Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2022, 3:33 PM

Fix clang-format issue.

iliya-diyachkov marked 9 inline comments as done.Mar 8 2022, 2:34 AM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp:532:3: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]

The warning was fixed.
Also addNumImm function was corrected.

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

arsenm added inline comments.Mar 15 2022, 7:29 AM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp
1407

No else after return

llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h
23

SPIRV namespace?

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

the static_cast to function is also no good since this could be a constexpr cast

llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
151

Use cast here and remove the assert below?

160

no const_casts?

llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h
23

Probably you mean

namespace llvm {
namespace SPIRV {
namespace Capability {
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
160

Module is fixed, but I'm not sure how to cast const Type *Ty to Type * without const_cast.

I see that other targets uses this approach in the similar cases (ARM, Hexagon).

I think the issues are fixed.

iliya-diyachkov marked 5 inline comments as done.Mar 15 2022, 6:36 PM

The format issue is fixed.

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

MaskRay added inline comments.Mar 27 2022, 10:32 AM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h
26

namespace XXX { enum XXX { ... } } can usually be replaced with enum class XXX {...}`

iliya-diyachkov marked an inline comment as done.Mar 27 2022, 7:33 PM
iliya-diyachkov added inline comments.
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h
26

Thanks, @MaskRay. The code is corrected.

arsenm added inline comments.Mar 28 2022, 1:57 PM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp
283

I think these all these switch functions need to move the unknown case to default and use llvm_unreachable after the switch to avoid warnings on all compilers

llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h
521

Don't need llvm::

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

getSubtarget<SPIRVSubtarget>.I think it's worthwhile to just have this as a permanent member of SPIRVCallLowering

164

This should still return false since multiple registers still can come from valid IR

172

This will still fail on constexpr casts and indirect calls. dyn_cast_or_null and return if you don't want to handle now

176

You should not construct one off MachineIRBuilders. Re-use the one passed in and adjust the insert point. There's additional state (like CSEInfo) you're losing

llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
339

Why all the null checks on Type? I would expect all of these to require non-null inputs

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
205–208

No, I mean MachineIRBuilder is a heavy class that should have state set up and maintained. It's not something to construct for a single use. The other selectors mostly just use BuildMI directly during selection.

iliya-diyachkov marked 3 inline comments as done.Mar 29 2022, 12:41 PM
iliya-diyachkov added inline comments.
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
37

Well, it looks reasonable.

172

Thanks, it'll be fixed.

176

Sure, this is bad code.

llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
339

Thanks, will fix.

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
205–208

Thanks, now I see that only AMDGPU and Mips targets sometimes use MachineIRBuilder. I'll change it in SPIRV.

The issues found by Matt have been resolved.

iliya-diyachkov marked 6 inline comments as done.Mar 31 2022, 2:02 AM
llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
205–208

I have left only one (relatively rare) case of using MachineIRBuilder in SPIRVInstructionSelector. I suppose this is acceptable since AMDGPU and Mips do it in a similar way.

iliya-diyachkov updated this revision to Diff 419967.EditedApr 2 2022, 4:39 AM

Extra headers in SPIRVUtils.h were removed. The changes in SPIRVUtils from the 6th patch were moved to the 5th patch.

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

Cosmetic changes in SPIRVGlobalRegistry.cpp and SPIRVUtils.h.

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

LGTM with nits

llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp
284

the llvm_unreachable is a bit pointless without a break here

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
90–92

getParamAlignment would be nicer

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
205–208

I've been meaning to fix those; I wouldn't use that as a good example.

811

Braces

996

Braces. Line wrapping is a bit weird looking

1041

Not much point asserting on things the verifier checks

This revision is now accepted and ready to land.Apr 5 2022, 5:14 PM
iliya-diyachkov added inline comments.Apr 6 2022, 5:03 AM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp
284

I see, will change to break.

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
90–92

It'll be corrected.

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
205–208

Ok, if you don't mind I'll add FIXME for this last case for now and fix it as soon as possible. This fix may require an additional extension in SPIRVGlobalRegistry, in which case I would commit it after this initial series of patches.

1041

Will remove.

The last issues have been fixed.

iliya-diyachkov marked 5 inline comments as done.
iliya-diyachkov marked an inline comment as done.Apr 7 2022, 1:30 AM
iliya-diyachkov updated this revision to Diff 421349.EditedApr 7 2022, 3:09 PM

Fix a warning in SPIRVCallLowering.cpp.

MaskRay added inline comments.Apr 7 2022, 11:35 PM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp
381

Can we use X macros for the long list?

487

Can we use X macros for the long list?

557

Can we use X macros for the long list?

587

Can we use X macros for the long list?

llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
48–50

make_unique

iliya-diyachkov added inline comments.Apr 8 2022, 8:05 AM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp
381

Initially all these functions were generated using macros. Then Renato suggested getting rid of them and we agreed to use expanded macros for now. Later we'll re-implement all the functions with tablegen (this work is in progress so I expect it to be completed in several months).

I have already fixed a number of issues in these functions after expanding the macros. I'm not sure that now we really need to replace them with new macros.

MaskRay added inline comments.Apr 8 2022, 10:03 AM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp
381

Ah, ok. @rengolin I feel that X macros may be more readable and less error-prone than open coding here.

TableGen can generate such strings as well, but it will add one step, bring a little abstraction code, and is probably not conciser than macros here (AIUI one line for one enumeration value). WDYT?

MaskRay added inline comments.Apr 8 2022, 10:05 AM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp
381

The recent clang-format discussion is also relevant here. I think the current open coding can be perceived as error-prone by some folks:
https://discourse.llvm.org/t/enable-single-line-case-statements-in-llvm-clang-format-style/61062

iliya-diyachkov added inline comments.Apr 8 2022, 5:46 PM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp
381

TableGen can generate such strings as well, but it will add one step, bring a little abstraction code, and is probably not conciser than macros here

As I know we'll have all these functions fully generated by TableGen, so it will be quite concise.

In the first version we had quite complex macros for all BaseInfo entities, and even more complex macros in the original Khronos repository, so we shouldn't return to that implementation.

I assume that you are now suggesting using very simple macros like this:

#define CASE(CLASS, MEMBER) \
  case CLASS::MEMBER: return #MEMBER;
...
StringRef SPIRV::getExecutionModeName(ExecutionMode e) {
  switch (e) {
  CASE(ExecutionMode, Invocations)

I'm not sure whether we need this at the moment, so I set TODO before all the functions for now.

However if you think it's a better solution then we have now, no problem, I'll change all the functions (with this open coding). But we all need to agree on this.

Fix the issues.

iliya-diyachkov marked 7 inline comments as done.Apr 8 2022, 6:00 PM
rengolin added inline comments.Apr 13 2022, 7:30 AM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp
20

why not use:

namespace llvm {
   namespace SPIRV {

and then avoid prefixing all types with SPIRV?

381

I honestly can't remember what was there before, but this huge list of strings does seem a bit out of hand.

With macros, we'd still have a huge list of calls, but we wouldn't have the opportunity to make typo mistakes (if using X(str) case str: return "#str";).

I'm also not sure why you need all of these strings at this level, unless you want to print all that in your assembly output. Is that the point? Or is it just debug?

llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp
20

You are right, we can avoid SPIRV prefix.

381

With macros, we'd still have a huge list of calls, but we wouldn't have the opportunity to make typo mistakes (if using X(str) case str: return "#str";).

Ok, I'll change this to X macros.

I'm also not sure why you need all of these strings at this level, unless you want to print all that in your assembly output. Is that the point?

Yes, we need to print all these operands in the assembly output. Anyway the TableGen version is in progress, so this huge list will be removed.

Remove SPIRV prefixing, use "X" macros to avoid typo mistakes in SPIRVBaseInfo.cpp.

iliya-diyachkov marked 2 inline comments as done.Apr 13 2022, 6:35 PM
rengolin accepted this revision.Apr 14 2022, 1:57 AM

Thanks @iliya-diyachkov, it looks cleaner, even if still very long. A table generated output will hopefully fix this soon.

First, sorry for the confusion if I made you do something only to revert later, but that shows the value of multi-person reviews. So, also thanks others for picking up on that.

I have no further comments, this looks good to me now. Thanks!

Thanks, Renato!

First, sorry for the confusion if I made you do something only to revert later, but that shows the value of multi-person reviews. So, also thanks others for picking up on that.

No problem. The initial version of macros was more complicated, so it was not just a return, but an adaptation of the original code.

Now we have to finish the review of the 6th patch. All the issues were fixed about a week ago, I hope Matt will respond soon.

MaskRay accepted this revision.Apr 14 2022, 3:04 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.
nikic added a subscriber: nikic.Apr 20 2022, 2:44 AM
nikic added inline comments.
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
287

This is incompatible with opaque pointers (https://llvm.org/docs/OpaquePointers.html). Please remove this call or revert the patch if this is non-trivial.

zuban32 added inline comments.
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
287

What if we use PointerType::getNonOpaquePointerElementType() instead? IMO that should do the trick while SPIR-V has no support of opaque pointers yet so we're only oging non-opaque way here.

nikic added inline comments.Apr 20 2022, 12:20 PM
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
287

You can use getNonOpaquePointerElementType(), but you need to implement a separate code-path for isOpaquePointerTy() if you do so.

llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
287

Sure, we'll implement. Thanks, Nikita.