Page MenuHomePhabricator

zuban32 (Aleksandr Bezzubikov)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 29 2021, 6:43 AM (60 w, 3 d)

Recent Activity

Thu, Jun 23

zuban32 added reviewers for D128471: [SPIR-V] Introduce SPIR-V global entities tracking and deduplication infra.: iliya-diyachkov, MaskRay, rengolin, arsenm, konrad.trifunovic, mpaszkowski, andreytr.
Thu, Jun 23, 3:15 PM · Restricted Project, Restricted Project
zuban32 requested review of D128471: [SPIR-V] Introduce SPIR-V global entities tracking and deduplication infra..
Thu, Jun 23, 3:11 PM · Restricted Project, Restricted Project

May 17 2022

zuban32 accepted D125404: [SPIRV] Add simple tests to improve test coverage.

LGTM

May 17 2022, 8:28 AM · Restricted Project, Restricted Project

May 16 2022

zuban32 added inline comments to D125404: [SPIRV] Add simple tests to improve test coverage.
May 16 2022, 1:39 PM · Restricted Project, Restricted Project
zuban32 added inline comments to D125404: [SPIRV] Add simple tests to improve test coverage.
May 16 2022, 10:35 AM · Restricted Project, Restricted Project

May 11 2022

zuban32 requested changes to D125404: [SPIRV] Add simple tests to improve test coverage.

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?

May 11 2022, 2:28 PM · Restricted Project, Restricted Project

Apr 20 2022

zuban32 added a reviewer for D116464: [SPIRV 5/6] Add LegalizerInfo, InstructionSelector and utilities: nikic.
Apr 20 2022, 8:57 AM · Restricted Project, Restricted Project

Feb 14 2022

zuban32 added a comment to D116465: [SPIRV 6/6] Add the module analysis pass and the simplest tests.

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

Feb 14 2022, 2:11 PM · Restricted Project, Restricted Project

Feb 9 2022

zuban32 added inline comments to D116465: [SPIRV 6/6] Add the module analysis pass and the simplest tests.
Feb 9 2022, 4:35 PM · Restricted Project, Restricted Project

Feb 8 2022

zuban32 added inline comments to D116465: [SPIRV 6/6] Add the module analysis pass and the simplest tests.
Feb 8 2022, 7:18 AM · Restricted Project, Restricted Project

Feb 3 2022

zuban32 added a comment to D116465: [SPIRV 6/6] Add the module analysis pass and the simplest tests.

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.

Feb 3 2022, 8:49 AM · Restricted Project, Restricted Project

Feb 2 2022

zuban32 added a comment to D116465: [SPIRV 6/6] Add the module analysis pass and the simplest tests.

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?

Feb 2 2022, 2:38 PM · Restricted Project, Restricted Project
zuban32 added a comment to D116465: [SPIRV 6/6] Add the module analysis pass and the simplest tests.

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:

Feb 2 2022, 1:52 PM · Restricted Project, Restricted Project
zuban32 added a comment to D116465: [SPIRV 6/6] Add the module analysis pass and the simplest tests.

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

Feb 2 2022, 9:49 AM · Restricted Project, Restricted Project
zuban32 added a comment to D116465: [SPIRV 6/6] Add the module analysis pass and the simplest tests.

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

Feb 2 2022, 8:43 AM · Restricted Project, Restricted Project

Jan 11 2022

zuban32 added inline comments to D115786: [SPIR-V 2/6] Add SPIRV{InstrFormats,InstrInfo,RegisterInfo,RegisterBanks...}.td.
Jan 11 2022, 2:04 PM · Restricted Project, Restricted Project

Jan 10 2022

zuban32 added inline comments to D116464: [SPIRV 5/6] Add LegalizerInfo, InstructionSelector and utilities.
Jan 10 2022, 12:14 PM · Restricted Project, Restricted Project
zuban32 added inline comments to D116464: [SPIRV 5/6] Add LegalizerInfo, InstructionSelector and utilities.
Jan 10 2022, 11:47 AM · Restricted Project, Restricted Project
zuban32 added inline comments to D116464: [SPIRV 5/6] Add LegalizerInfo, InstructionSelector and utilities.
Jan 10 2022, 11:45 AM · Restricted Project, Restricted Project
zuban32 added inline comments to D116464: [SPIRV 5/6] Add LegalizerInfo, InstructionSelector and utilities.
Jan 10 2022, 10:39 AM · Restricted Project, Restricted Project

Dec 31 2021

zuban32 updated subscribers of D116462: [SPIRV 3/6] Add MC layer, object file support and InstPrinter.

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?

Dec 31 2021, 6:36 PM · Restricted Project, Restricted Project

Sep 21 2021

zuban32 added a comment to D108375: [GlobalISel] Support ConstantAsMetadata in IRTranslator.

Ping

Sep 21 2021, 7:07 AM · Restricted Project

Sep 17 2021

zuban32 added a comment to D108375: [GlobalISel] Support ConstantAsMetadata in IRTranslator.

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

Sep 17 2021, 7:27 AM · Restricted Project

Sep 15 2021

zuban32 updated the diff for D108375: [GlobalISel] Support ConstantAsMetadata in IRTranslator.

Rebased, added a test.

Sep 15 2021, 4:58 PM · Restricted Project

Sep 2 2021

zuban32 added a comment to D108375: [GlobalISel] Support ConstantAsMetadata in IRTranslator.

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

Sep 2 2021, 12:49 PM · Restricted Project

Aug 26 2021

zuban32 abandoned D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..
Aug 26 2021, 9:45 AM · Restricted Project
zuban32 added a comment to D108375: [GlobalISel] Support ConstantAsMetadata in IRTranslator.

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

Aug 26 2021, 9:45 AM · Restricted Project
zuban32 added a comment to D108375: [GlobalISel] Support ConstantAsMetadata in IRTranslator.

Gentle ping

Aug 26 2021, 7:48 AM · Restricted Project

Aug 25 2021

zuban32 updated the diff for D108375: [GlobalISel] Support ConstantAsMetadata in IRTranslator.

Fix clang-tidy warning

Aug 25 2021, 4:48 PM · Restricted Project

Aug 20 2021

zuban32 added a comment to D108375: [GlobalISel] Support ConstantAsMetadata in IRTranslator.

Can you add a testcase?

Aug 20 2021, 4:50 AM · Restricted Project

Aug 19 2021

zuban32 added reviewers for D108375: [GlobalISel] Support ConstantAsMetadata in IRTranslator: paquette, aemerson, arsenm.
Aug 19 2021, 7:47 AM · Restricted Project
zuban32 requested review of D108375: [GlobalISel] Support ConstantAsMetadata in IRTranslator.
Aug 19 2021, 7:44 AM · Restricted Project

Jul 13 2021

zuban32 added a comment to D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..

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

Jul 13 2021, 12:13 PM · Restricted Project

Jul 8 2021

zuban32 added a comment to D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..

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

Jul 8 2021, 5:08 PM · Restricted Project

May 21 2021

zuban32 added a comment to D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..

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

May 21 2021, 7:23 AM · Restricted Project

May 19 2021

zuban32 added a comment to D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..

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

May 19 2021, 8:50 AM · Restricted Project

May 18 2021

zuban32 added a comment to D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..

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?

May 18 2021, 11:35 AM · Restricted Project

May 17 2021

zuban32 added a comment to D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..

@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

May 17 2021, 9:11 AM · Restricted Project

May 13 2021

zuban32 added a comment to D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..

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?

May 13 2021, 3:18 PM · Restricted Project

May 11 2021

zuban32 added a comment to D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..

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.

May 11 2021, 12:34 PM · Restricted Project
zuban32 added a comment to D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..

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

May 11 2021, 8:58 AM · Restricted Project

May 7 2021

zuban32 added a comment to D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..

Gentle ping

May 7 2021, 3:35 PM · Restricted Project

Apr 29 2021

zuban32 added reviewers for D101538: [GlobalISel][IRTranslator] Make translate() methods virtual.: aemerson, qcolombet, t.p.northover, paquette, foad.
Apr 29 2021, 5:18 PM · Restricted Project
zuban32 updated the diff for D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..

Rebased

Apr 29 2021, 11:54 AM · Restricted Project
zuban32 requested review of D101538: [GlobalISel][IRTranslator] Make translate() methods virtual..
Apr 29 2021, 7:40 AM · Restricted Project