[PATCH] D116465: [SPIRV 6/6] Add 2 essential passes and the simplest tests

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov added a comment.

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

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

In fact we do map types, constants, global vars and external function decls but at local function level (however I did not include this implementation to this series, replacing this with simple type/const instruction copying in extractInstructionsWithGlobalRegsToMetablockForMBB for now, in the next patches it will be changed to the map using). In llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h you can find the interface getOrCreateSPIRVType, which returns either existing OpType*** instruction or create new one. Then in GlobalTypesAndRegNumPass we walk the map and create only required type/const/var/fdecl instructions in the "global" function.
Anyway in this approach  we need to leave the instructions' copies in regular functions to pass the verifier (in LLVM build with -DLLVM_ENABLE_EXPENSIVE_CHECKS=on -DLLVM_ENABLE_ABI_BREAKING_CHECKS=on).

Taking into account the implemented mapping now we keep duplicates at function level to make MIR consistent. For 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
    ...

After emitting:

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

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116465/new/

https://reviews.llvm.org/D116465