OK, LGTM then

Could you pls specify a list of these small bug fixes, at the very high level at least? An ideal variant would be to not mix them up with this regularization stuff at all.

• clang-format

• Address comments

LGTM regarding removal of unrelated SPIR-V code

In D129730#3669819, @iliya-diyachkov wrote:

In D129730#3668881, @zuban32 wrote:

To me it seems as an unnecessarily huge change. Can't it be split, at least keep PrepareFunctions and other minor changes separate?

Well, it looks big, but comparable to the patches from the initial series (and even smaller then some of them). By the way, the changes in most files are quite minor. Only 5 files are significantly changed (SPIRVAsmPrinter.cpp, SPIRVCallLowering.cpp, SPIRVGlobalRegistry.cpp, SPIRVInstructionSelector.cpp, SPIRVModuleAnalysis.cpp), and just one new pass is added.

It's well tested and provides a good improvement in the stability and functionality of the SPIRV backend, so I wanted to introduce it before the 15th release. If the size is not a big issue and you don't mind, I will commit it as it is.

To me it seems as an unnecessarily huge change. Can't it be split, at least keep PrepareFunctions and other minor changes separate?

@iliya-diyachkov FYI there're 6 more passing tests which are not upstreamed yet, but I've not added them here since they're irrelevant to the current changes

• Fix potential failure, add 2 newly passed LITs

Restore

Fix potential failure, add 2 newly passed LITs

LGTM

To me this subset of tests looks very counter-intuitive, I'd strongly prefer to have LITs related commits formed by some tests' common properties, e.g. '[SPIR-V] Add simple transcoding tests'. What was your motivation behind this particular subset, are these the only tests currently passing?

In D116465#3320801, @iliya-diyachkov wrote:

@arsenm, when you suggested moving GlobalTypesAndRegNum' operations to AsmPrinter, did you implied that we need to insert SPIRVDummyGlobalPass after all SPIRVGISel passes to have whole MIR at the first run of AsmPrinter? Or is there another approach (or some pass configuration) to achieve the correct processing of the whole MIR in AsmPrinter?

In D116465#3293196, @rengolin wrote:

In D116465#3293156, @iliya-diyachkov wrote:

And as far as I understand your suggestion this de-duplicated variant will break the verifier (after generation):

func1
  %0 = OpTypeInt 32
  %1 = OpTypeInt 64
  ...
  %4 = OpIAdd %1 %2 %3
  ...
func2
  // (no definition of %1)
  ... 
  %4 = OpIMul %1 %2 %3
  ...

No, I'm suggestion you emit on every function (as is functionally correct) but reuse the same ID, and keep that same ID on global_function.

The singleton is just caching the VReg numbers, not the actual MNodes.

Code emission will be the same as what you have today after that.

I agree that this is a natural desire to use some caching in this situation. But I join Alexander in the question how the proposed caching would work at module (cross-function) level? As I understand we can define %type1 register in func1 and refer to the register inside func1, but it's illegal to refer %type1 in func2 without a local definition of %type1 (let me remind that in SPIR-V the type definition is a regular MIR instruction which produces %type register).

By just re-declaring each used type/constant in each subsequent function with the same VReg ID.

In your example:

After generation:

func1
  %0 = OpTypeInt 32
  %1 = OpTypeInt 64
  ...
  %4 = OpIAdd %1 %2 %3
  ...
func2
  %0 = OpTypeInt 32
  %1 = OpTypeInt 64
  ...
  %4 = OpIMul %1 %2 %3
  ...

After GlobalTypesAndRegNumPass:

global_func:
  %0 = OpTypeInt 32
  %1 = OpTypeInt 64
...
func1
  %0 = OpTypeInt 32
  %1 = OpTypeInt 64
  ...
  %4 = OpIAdd %1 %2 %3
  ...
func2
  %0 = OpTypeInt 32
  %1 = OpTypeInt 64
  ...
  %4 = OpIMul %1 %2 %3
  ...

To have the same MIR in your second example, straight from generation, you have to follow these steps:

• First time you see, generate a new type (OpTypeInt 32) with a new auto-increment ID, cache the ID for the original type, emit on global_func and *also* on the function you're generating.
• Every other time you see, get the cached VReg ID and re-emit (a new MNode) using that same ID.

In your example:

• Inside func1 you generate a new type (OpTypeInt 32) and call it %0, cache %0 for i32, emit on global_func and *also* on func1.
• Inside func2 you get the cached version (of i32 as %0) and generate it *again* only on func2 (on a new Node, using %0). global_func already have it's the same VReg number on all previous functions.

If you use a global auto-increment next_id(), there's no way VReg IDs for operations in any function will clash with previously defined types and constants (in previous functions).

The VReg numbers on the later functions will be (numerically) large, but the number of different IDs in each function will still be the same, so code generation should be equally efficient.

In D116465#3292037, @rengolin wrote:

I didn't mean the duplicated types/constants would survive into codegen, but I get your meaning, it would have to be commoned up at some point.

So, what about a singleton emitter for types and constants?

Instead of just get_next_id() you could have a get_type(...) and get_constant(...) that returns a cached MIR node if it has created a similar one already, or creates a new one, caches and returns it.

In this way, there's only ever one copy of each and they're only generated once, with no need to deduplicate.

The only issue with this implementation is that types, constants, functions, instructions will all have intertwined IDs, but never refer to larger IDs, by design.

When emitting "int foo()", if int hadn't been emitted before, it'd get the next id (say, %23) and then foo would get %24 and would refer to %23 as its type.

Eventually, all types used in the program would have been emitted already and be brought up from the cache every time.

This way, there's no need to renumber or common up and the space is still optimal.

What am I missing?

In D116465#3291049, @rengolin wrote:

In D116465#3290957, @zuban32 wrote:

Then let me describe the problem in more details, I'd like us all to be on the same page with this.

Awesome! This is much clearer to me, thanks!

The latter seems to be a minor problem - the reason for it to exist is SPIRV binary format requiring global numbering of all the defined values, i.e. if there're 2 functions each containing only 1 instruction then these instructions can't be both numbered %0.

Do you have a maximum ID? If not, than in theory, you just need a static global auto-increment ID (like a get_next_id() on some target emission class) and off you go emitting globally unique IDs.

The former one attempts to solve a bigger problem. Basically the SPIRV binary is structured as following:

...
types, constants
...
functions definitions

I see, so your "global" function contain the type and constant declarations, that the functions use in their own declarations.

So, you're not "moving" anything, just creating a placeholder for those definitions and indexing them.

in our approach each MachineFunction has its own copies of type decls and constants (and other stuff like that) during the whole translation process, but eventually they need to be deduplicated and 'globalized'. That can be done either on MIR level or during binary emission, but it has done be done anyway.

IIUC, they're in each function because you generate for each function you pass through, so:

In D116465#3290818, @rengolin wrote:

In D116465#3290716, @zuban32 wrote:

@rengolin what would be considered as a proper implementation?

I don't think I have quite understood what the problem is, tbh.

If this is just some global value numbering, why is it being done so late (ie, not in IR)?

If this is a SPIR-V hardware requirement, that needs a specific guaranteed shape that cannot rely on the state of IR->MIR conversions (especially the generic parts), then there may not be another way out.

But I'll leave for others to express their opinions, as I'm not familiar with the problem.

In D116465#3289893, @rengolin wrote:

I'm still not convinced that these passes are the right way to go, but I won't block if others think it has merit, even if just temporary, until the proper implementation emerges.

We don't need these patches to be perfect, just good enough to generate some instructions and tell us that the direction the target is going is in sync with what we think should be.

If this is something that can be changed later, I'd be fine with it.

@arsenm @MaskRay

Iliya, since this is the 3rd patch of a total of 6, do I understand
correctly that the second one is https://reviews.llvm.org/D115786 from
Michal?

Ping

Someone pls commit this, I don't have an access

Rebased, added a test.

In D108375#2967518, @zuban32 wrote:

Can someone pls commit this? I don't have an access to do that myself

Can someone pls commit this? I don't have an access to do that myself

Gentle ping

Fix clang-tidy warning

In D108375#2955777, @paquette wrote:

Can you add a testcase?

Let me explain my problem with a short example. Assume we have the following IR:

@qcolombet @arsenm @foad this change is already abandoned, but my question seems directly related to it.

In D101538#2769796, @qcolombet wrote:

I wouldn't be so sure about that. One particular usecase I need this for is a bitcast between pointer types:

%1 = bitcast i8* %0 to %struct.ST*
In our target they're both of p0 LLTs, so the default method eliminates this instruction while in our target it's necessary to keep it.

That sounds like you're abusing the IR here. Shouldn't you have a different addrspace here?

FWIW, when we discussed the design of GISel internally originally we explicitly chose not to allow virtual overloading of the IR translation steps because that step follow what is represented in the IR.
To make a parallel with SDISel, this is as if we would allow SDISelBuilder to be target specific.

Bottom line, for just this one use case I feel that it doesn't make sense to allow virtual method everywhere yet.

You could workaround that with introducing a pseudo/intrisinc in codegen prepare for instance.

My 2 cents.

Cheers,
-Quentin

In D101538#2768105, @foad wrote:

Won't this problem go away when we introduce FP LLTs?

I wouldn't be so sure about that. One particular usecase I need this for is a bitcast between pointer types:

%1 = bitcast i8* %0 to %struct.ST*

In our target they're both of p0 LLTs, so the default method eliminates this instruction while in our target it's necessary to keep it.

What are you going to do when IR switches to opaque pointers? https://llvm.org/docs/OpaquePointers.html

In D101538#2764529, @arsenm wrote:

In D101538#2751818, @aemerson wrote:

In D101538#2750853, @zuban32 wrote:

In D101538#2745606, @aemerson wrote:

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Well, our usecase might not be that canonical, let me briefly explain. For example, translating a bitcast - default IRTranslator implementation turns it into a COPY when LLTs of src and dst are the same. In our target (SPIR-V) LLTs do not matter that much, the typeinfo is represented with some pseudo instructions, so we run into this situation quite often. And losing a bitcast in the translator doesn't work for us as it'd obviously be quite difficult to recover it.

Won't this problem go away when we introduce FP LLTs?

@aemerson @foad if no one has any idea how to make CRTP approach feasible here, then I'd suggest to merge this, I have no write access though

In D101538#2753323, @foad wrote:

In D101538#2751818, @aemerson wrote:

In D101538#2750853, @zuban32 wrote:

In D101538#2745606, @aemerson wrote:

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Well, our usecase might not be that canonical, let me briefly explain. For example, translating a bitcast - default IRTranslator implementation turns it into a COPY when LLTs of src and dst are the same. In our target (SPIR-V) LLTs do not matter that much, the typeinfo is represented with some pseudo instructions, so we run into this situation quite often. And losing a bitcast in the translator doesn't work for us as it'd obviously be quite difficult to recover it.

I see. Is this a widespread issue with a lot of the translation functions or just bitcasts? If it's just bitcast you could just make that virtual so we don't have to pay the virtual function call overhead every translate. If not, then I'm ok with doing this.

How about doing it with https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern#Static_polymorphism to avoid the virtual function call overhead?

In D101538#2751818, @aemerson wrote:

In D101538#2750853, @zuban32 wrote:

In D101538#2745606, @aemerson wrote:

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Well, our usecase might not be that canonical, let me briefly explain. For example, translating a bitcast - default IRTranslator implementation turns it into a COPY when LLTs of src and dst are the same. In our target (SPIR-V) LLTs do not matter that much, the typeinfo is represented with some pseudo instructions, so we run into this situation quite often. And losing a bitcast in the translator doesn't work for us as it'd obviously be quite difficult to recover it.

I see. Is this a widespread issue with a lot of the translation functions or just bitcasts? If it's just bitcast you could just make that virtual so we don't have to pay the virtual function call overhead every translate. If not, then I'm ok with doing this.

In D101538#2745606, @aemerson wrote:

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Gentle ping

Rebased