Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hoist resource variables using a global initializer function correctly #4894

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft
30 changes: 25 additions & 5 deletions source/slang/slang-ir-legalize-types.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -343,8 +343,20 @@ struct LegalCallBuilder
// result type of the function, so we know that
// the legalization funciton/call will use a `void`
// result type.
//
_emitCall(m_context->builder->getVoidType());

// If our call is in global scope (initializer) we should
// hoist the call into the start of every entry-point
if (!m_context->builder->getInsertLoc().getInst()->getParent()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

|| m_context->builder->getInsertLoc().getInst()->getParent()->getOp() == kIROp_Module)
{
for (auto i : m_context->getEntryPoints())
{
m_context->builder->setInsertBefore(i->getFirstOrdinaryInst());
_emitCall(m_context->builder->getVoidType());
}
}
else
_emitCall(m_context->builder->getVoidType());
return resultVal;
}
break;
Expand All @@ -370,11 +382,19 @@ struct LegalCallBuilder
auto simpleType = resultType.getSimple();
auto builder = m_context->builder;

// Recall that a local variable in our IR represents a *pointer*
// Recall that a variable in our IR represents a *pointer*
// to storage of the appropriate type.
//
auto varPtr = builder->emitVar(simpleType);

IRInst* varPtr = nullptr;
if (m_call->parent->getOp() == kIROp_Module)
{
// If we were going to emit an IRVar in global scope, emit a GlobalVar instead
varPtr = builder->createGlobalVar(simpleType);
}
else
{
varPtr = builder->emitVar(simpleType);
}
// We need to pass that pointer as an argument to our new
// `call` instruction, so that it can receive the value
// written by the callee.
Expand Down
16 changes: 16 additions & 0 deletions source/slang/slang-legalize-types.h
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,22 @@ struct IRTypeLegalizationContext

List<PointerValue> activePointerValues;

List<IRFunc*> entryPoints;

List<IRFunc*>& getEntryPoints()
{
if (entryPoints.getCount() == 0)
{
for (auto i : module->getGlobalInsts())
{
if (auto func = as<IRFunc>(i))
if (i->findDecoration<IREntryPointDecoration>())
entryPoints.add(func);
}
}
return entryPoints;
}

IRBuilder* getBuilder() { return builder; }

/// Customization point to decide what types are "special."
Expand Down
48 changes: 48 additions & 0 deletions tests/compute/type-legalize-global-with-init-function.slang
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// type-legalize-global-with-init.slang
//
// Confirm that type legalization can handle a global constant
// with a resource type or a type that recursively contains
// resources.
//
//TEST(compute):COMPARE_COMPUTE(filecheck-buffer=BUF): -shaderobj
//
//TEST_INPUT:ubuffer(data=[0 0 0 0], stride=4):out,name=outputBuffer
RWStructuredBuffer<uint> outputBuffer;

//TEST_INPUT:ubuffer(data=[1 2 3 4 5 6 7 8], stride=4):name=inputBuffer
RWStructuredBuffer<uint> inputBuffer;

static const RWStructuredBuffer<uint> gBuffer = inputBuffer;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@ArielG-NV ArielG-NV Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


struct Stuff
{
__init(RWStructuredBuffer<uint> inA, RWStructuredBuffer<uint> inB)
{
a = inA;
b = inB;
}
RWStructuredBuffer<uint> a;
RWStructuredBuffer<uint> b;
}

static const Stuff gStuff = Stuff( inputBuffer, inputBuffer );

uint test(uint x)
{
return gBuffer[x]
+ gStuff.a[x + 1] * 16
+ gStuff.b[x + 2] * 256;
}

[numthreads(4, 1, 1)]
void computeMain(uint3 dispatchThreadID : SV_DispatchThreadID)
{
let tid = dispatchThreadID.x;
let inVal = tid;
let outVal = test(inVal);
outputBuffer[tid] = outVal;
}
//BUF: 321
//BUF: 432
//BUF: 543
//BUF: 654
7 changes: 6 additions & 1 deletion tests/compute/type-legalize-global-with-init.slang
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// with a resource type or a type that recursively contains
// resources.
//
//TEST(compute):COMPARE_COMPUTE: -shaderobj
//TEST(compute):COMPARE_COMPUTE(filecheck-buffer=BUF): -shaderobj
//
//TEST_INPUT:ubuffer(data=[0 0 0 0], stride=4):out,name=outputBuffer
RWStructuredBuffer<uint> outputBuffer;
Expand Down Expand Up @@ -37,3 +37,8 @@ void computeMain(uint3 dispatchThreadID : SV_DispatchThreadID)
let outVal = test(inVal);
outputBuffer[tid] = outVal;
}
//BUF: 321
//BUF: 432
//BUF: 543
//BUF: 654

This file was deleted.

Loading