WGSL: Fix issue where global calls are generated

[aleino-nv]

• Refactor the SPIR-V inlining legalization code so that it's reusable by other targets
• Apply said legalization code for WGSL
• Extend the legalization code such that globals left unused after inlining are removed

The last step (also the last commit) gets rid of the global call and fixes the issue.

#5607

[aleino-nv]

/format

[slangbot]

🌈 Formatted, please merge the changes from this PR

[aleino-nv]

/format

[slangbot]

🌈 Formatted, please merge the changes from this PR

[aleino-nv]

Some tests were failing:

• tests/spirv/global-compute.slang fails for SPIR-V, seemingly because OpEntryPoint is removed
  • I have a hacky workaround and a TODO comment for this, at the moment.
  • It would be good to understand why things fail for SPIR-V without that workaround...
• Some WGSL tests failed because the inlining code produced IRGlobalValueRef which were not handled during emit.
  • I added some code for this

[aleino-nv]

/format

[slangbot]

🌈 Formatted, please merge the changes from this PR

[aleino-nv]

/format

[slangbot]

🌈 Formatted, please merge the changes from this PR

[aleino-nv]

/format

[slangbot]

🌈 Formatted, please merge the changes from this PR

[aleino-nv]

I got a test timeout...

  internal/renderpasses/test_InternalPathTracerVolumeTransmittance_vulkan : FAILED (600.0 s)
    Process killed due to timeout
    View test at: http://SlangWin5:8080//unknown/internal/renderpasses/test_InternalPathTracerVolumeTransmittance_vulkan

[csyonghe]

This is a band aid instead of an actual fix.

As we discussed in the meeting, the true problem is when we clone the global SpirvASM inst to a function, we didn't update the operand references to point to the cloned version of the local spirv operand insts, as a result, the inlined asm block is still referencing the child insts in the original global asm block.

The fix here just prevents removal of the global asm block, but the right fix is to make sure we dont have such inter block references back to the original global asm block.

[aleino-nv]

Ok, I must have misunderstood something.

Yesterday I stepped through the inlining code in the debugger and noticed that the SPIRVAsmOperandInsts didn't get inlined (unlike all of the other operands) and so there was no clone to be found.

I figured this is something that can happen in general, and so we must check for uses recursively before deleting the global inst.

So you are saying that the existing inlining logic is broken?

I will take another look.

[aleino-nv]

@csyonghe Should the fix be that, when we decide to inline SPIRV ASM insts then we force-inline things it contains, like the SPIRVAsmOperandInsts etc.. ?

[csyonghe]

When we looked through the code together, it appears to me that the SPIRVAsm parent inst is being cloned, and its children are also being cloned. It is just the operands of the clone seems to be still referencing the original ones instead of the cloned ones.

e.g.

%b = SPIRV_Asm {
    %x = SPIRVAsmOperandInst ...
    %y = SPIRVAsmOperandInst ...
     %z = SPIRVAsmInst %x %y
}

got cloned into main as:

func %main  {
%block = block {
    %b' = SPIRV_Asm {
       %x' = SPIRVAsmOperandInst ...
       %y' = SPIRVAsmOperandInst ...
        %z' = SPIRVAsmInst %x %y // !!! should be %x' here, instead of %x
    }
}
}

[aleino-nv]

Oh, I think I see now!

The SPIRVAsmOperands are special in that they are parented to the SPIRVAsm which we would like to delete.

The inlineInst function does indeed inline its children, but then later when we get to the last child which is a SPIRVAsmInst taking SPIRVAsmOperands as operands, we decide not to inline them.

I see that there is a dictionary called m_mapGlobalInstToShouldInline that is first consulted.
Perhaps whenever we inline some inst we should set it as true in m_mapGlobalInstToShouldInline...

[aleino-nv]

Oh, I think I see now!

The SPIRVAsmOperands are special in that they are parented to the SPIRVAsm which we would like to delete.

The inlineInst function does indeed inline its children, but then later when we get to the last child which is a SPIRVAsmInst taking SPIRVAsmOperands as operands, we decide not to inline them.

I see that there is a dictionary called m_mapGlobalInstToShouldInline that is first consulted. Perhaps whenever we inline some inst we should set it as true in m_mapGlobalInstToShouldInline...

That works, but only because the SPIRVAsmInst taking these SPIRVAsmOperandInst's as operands happen to appear after the SPIRVAsmOperandInst's children of the SPIRVAsm...

If I instead mark m_mapGlobalInstToShouldInline to true for any inlined insts first thing in inlineInst, and then I can change shouldInlineInstImpl to return true in case any ancestors of the inst in question should be inlined according to m_mapGlobalInstToShouldInline.

[aleino-nv]

I did something similar to the above, and it seems to work.

[aleino-nv]

/format

[slangbot]

🌈 Formatted, please merge the changes from this PR

[csyonghe]

Looks good to me.