Hoist resource variables using a global initializer function correctly

[ArielG-NV]

Fixes: #4874
Goal:
Hoist resource variables using a global initializer function in such a way that any global with initializer function has any possible resource variables hoisted into their use location directly (otherwise many targets won't work).
Changes:

1. If legalize a global kIROp and it creates a global, make a GlobalVar

[only done for resource variables for simplicity of legalization pass]

2. Add a legalization pass that moves any global GlobalVar stores (initializers) into the GlobalVar init block
3. If we have a GlobalVar which is computed to be an alias (direct assignment to another variable), hoist the alias's value into IRGlobalVar use-sites.

[csyonghe]

I understand what this is trying to achieve here, but I think this is likely not the right way.

Usually if you have a global variable and you want to initialize it with some code (e.g. a call), you create a IRGlobalVar, which is a IRGlobalValueWithCode that can have a body just like a IRFunc. Inside the body you create a basic block with instructions + a return instruct that returns the value of the init expr.

Here instead of inserting a call into global scope, we should be creating that IRGlobalVar, and rely on follow up passes to insert the logic in the body into entry points.

[csyonghe]

In general I think we will have issues when there are global resource variables that is being written to dynamically, so I am not sure if this is something we want to allow...

static const currently is overloaded to also mean compile time constants, maybe we should disallow static consts of resource type as well. Is this test coming from some existing user code?

[ArielG-NV]

In general I think we will have issues when there are global resource variables that is being written to dynamically, so I am not sure if this is something we want to allow...

We will have an issue with dynamic resource variable accesses.
Slang is lacking a compiler pass to manage non-compile-time known resource accesses such as:

RWStructuredBuffer<uint> outputBuffer;
RWStructuredBuffer<uint> inputBuffer1;
RWStructuredBuffer<uint> inputBuffer2;
RWStructuredBuffer<uint> inputBuffer3;

[numthreads(4,4,4)]
void computeMain(uint3 dispatchThreadID : SV_DispatchThreadID)
{
    RWStructuredBuffer<uint> resource;
    if(dispatchThreadID.x == 1)
    {
        resource = inputBuffer1;
    }
    else if(dispatchThreadID.x == 2)
    {
        resource = inputBuffer2;
    }
    else if(dispatchThreadID.x == 3)
    {
        resource = inputBuffer3;
    }

    // This is illegal since resource is not known at compile time.
    outputBuffer[0] = resource[1];
}

From what I understand to add this would just piggy-back on our dynamic dispatch system logic:

1. Diagnose all non-constexpr resources, assign all resources of equal type a different ID (like with dynamic dispatch).
2. Any assignments to a resource assigns an ID (not the actual resource)
3. All accesses to a non-compile-time resource specializes all texture intrinsic uses to pick the correct texture using our ID (if/else binary tree)

[ArielG-NV]

static const currently is overloaded to also mean compile time constants, maybe we should disallow static consts of resource type as well. Is this test coming from some existing user code?

No test currently uses static const Stuff gStuff = Stuff( inputBuffer, inputBuffer );.
The problem is that some tests use static const Stuff gStuff = { inputBuffer, inputBuffer }, which once #4854 is merged will be an issue since we start emitting what is effectively static const Stuff gStuff = Stuff( inputBuffer, inputBuffer );.

[csyonghe]

Reading at the code here, it seems clear to me that we need to legalize out the resource typed global variable out of existence before we reach legalizeType, instead of letting legalizeType doing weird things such as inserting a store in global scope and then try to clean up that mess in a separate pass.

In this case, the global static const should have been lowered into a IRGlobalConstant whose value is an IRCall. We should then recognize these IRGlobalConstants and IRCalls and inline them before hitting legalization.

[ArielG-NV]

In this case, the global static const should have been lowered into a IRGlobalConstant whose value is an IRCall. We should then recognize these IRGlobalConstants and IRCalls and inline them before hitting legalization.

I will work on this solution 👍

Edit: As per conversation outside github-comments, work on this PR will halt for now.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}