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

Add slangc flag to -zero-initialize all variables #3987

Merged

Conversation

ArielG-NV
Copy link
Collaborator

@ArielG-NV ArielG-NV commented Apr 19, 2024

Adds -zero-initialize flag to set values to a __default() expression if they are missing an initExpr.
Fixes #3516

Adds `-zero-initialize` flag to set values to a __default() expression if they are missing a initExpr.
@ArielG-NV ArielG-NV changed the title Slangc flag to -zero-initialize all variables, #3516 Slangc flag to -zero-initialize all variables Apr 19, 2024
@ArielG-NV ArielG-NV changed the title Slangc flag to -zero-initialize all variables Add slangc flag to -zero-initialize all variables Apr 19, 2024
@@ -1636,6 +1636,23 @@ namespace Slang
}
}

if (!varDecl->initExpr &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this logic should be in SemanticsDeclBodyVisitor::checkVarDeclCommon instead.
There we are checking if a default ctor exists and calls it if so.

We should extend that logic by saying if default ctor does not exist, then initialize it with __default<T>().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We will not be-able todo this logic in BodyVisitor since then when setting init's to struct fields we would have already parsed+setup the default ctor body logic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be synthesizing ctor signature in header visitor, and synthesize the ctor body in body visitor, there we call the ctors of the field. Why isn't this possible? We shouln't be checking the init exprs in body visitor IMO.

@csyonghe
Copy link
Collaborator

You should write
Fixes #xxxx
Or
Closes #xxxx
For GitHub to automatically link the issue.

1. We must keep zero-initialize in SemanticsDeclHeaderVisitor. This is done because else a ctor will be initialized before we can set struct fields to `__default`.
2. IRDefaultCtorDecoration was added to track default ctor's with parent struct.
3. ParentAggTypeModifier was added to track ChildOfStruct->IRType for sharing data such as with functions. This is required to ensure we associate a lowered function with a lowered struct type
This was done since decorations are checked for IR objects, storing auxillary info does not work here as a result if usable object.
@@ -1585,6 +1585,24 @@ class DynamicUniformModifier : public Modifier
SLANG_AST_CLASS(DynamicUniformModifier)
};

// Sharing IRInst data of AggType (Struct/Interface) with members.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be needed. In general we should never be creating AST nodes that references IR constructs.

IR_LEAF_ISA(StructType)

IRFunc* defaultCtor = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't be adding fields to IR insts like this. All references must be encoded as either a child or an operand.

And IRStruct should not be referencing ctors directly. You can use a decoration to associate a default ctor if necessary. But i don't think we should need this, because we should be generating all default ctor calls during IR lowering, and we should never need to have the concept of a ctor in the IR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a hole in our generics system that we don't have to address in this PR.

What we probably should do now is that if T doesn't have a default initializer from any constraints, then we don't call any default ctor even after specialization. However, we still want to make sure we zero initialize whatever type it became after specialization with defaultConstruct inst.

So when we lower the generic function to IR, we check if T has default ctor, if it has we call it. If not we emit a defaultConstruct inst.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we do this, we don't need to keep track of the default ctor of each struct type in the IR in any explicit way.

Copy link
Collaborator Author

@ArielG-NV ArielG-NV Apr 22, 2024

Choose a reason for hiding this comment

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

The workaround added was not to handle generics, generics work as intended with __default as tested. I do see where this can be useful though (interfaces->struct to define an __init)

Passing a struct<->Ctor was done because of the cases like vector<...struct...>. We don't exactly know what is inside the container, so we cannot simply call an 'invoke' to the ctor. Only __default has emitting logic to manage these code edge cases which is why a ctor was passed through.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well,if you have:

struct MyContainer<T>
{
     T obj;
}

Then the right answer is always do obj = __default<T>(), which will eventually lower into defaultConstruct.

If you have:

struct MyContainer<T:IDefaultInitializable> // or any other interface that leads to `__init()` to be found
{
    T obj;
}

Then you lower it into:

obj = T();

Copy link
Collaborator

Choose a reason for hiding this comment

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

In either case, you shouldn't need to keep track of this additional ctor info.

{
for (auto ctor : decl->getMembersOfType<ConstructorDecl>())
{
auto parentStructTypeModifier = this->context->astBuilder->create<ParentStructTypeModifier>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can simply maintain a side band dictionary in the ir lowering context to keep track of struct types and their ctors.

//TEST_INPUT:ubuffer(data=[0], stride=4):out,name=outputBuffer
RWStructuredBuffer<int> outputBuffer;

__generic<T : __BuiltinArithmeticType>
Copy link
Collaborator

Choose a reason for hiding this comment

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

For generics, we will need a IDefaultInitializable builtin interface that has a __init() requirement.

Then for any generic type that conforms to IDefaultInitializable, we can just call the interface method to initialize it. By default, we make all builtin types conform to this interface, and we can make all user defined struct types to automatically conform to the interface as we synthesize the default constructor.

Copy link
Collaborator Author

@ArielG-NV ArielG-NV Apr 22, 2024

Choose a reason for hiding this comment

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

This would solve the problem we have with vectors and remove the need for a workaround

we could define the vector IDefaultInitializable as the base type __default, which would resolve into a ctor eventually for all elements.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, all builtin interfaces should inherit from IDefaultInitializable.

@jkwak-work
Copy link
Collaborator

I am wondering if we can have tests for cases when "-zero-initialize" is not used.
In game development, the uninitialized state is preferred over implicit initialization with zero values for the runtime performance reason.
I am little worried that we may accidently initialize variables to zero even when "-zero-initialize" is not used.

@csyonghe
Copy link
Collaborator

@jkwak-work Agreed that we need a test case to cover this.

Basically, when -zero-initialize is not specified, then we should not be generating default ctors if a type does not have any member initializers, when we should not be generating assignments in default ctors for members that are not initialized.

Then, if we don't synthesis such a default ctor, we won't be calling the default ctor at variable declaration sites.

@ArielG-NV
Copy link
Collaborator Author

I will add the test case, I agree that there should be one for each possible senario:

  1. default init generated due to field init expression
  2. no init should be generated due to 0 fields with init expressions

@ArielG-NV ArielG-NV changed the title Add slangc flag to -zero-initialize all variables WIP: Add slangc flag to -zero-initialize all variables Apr 22, 2024
Since `IDefaultInitializable` is taking a considerabley larger amount of time than anticipated I am pushing some of the other fixes requested. I did not remove the "IRStruct storing a default Ctor" hack yet.

mostly renamed/adjusted tests to work as intended

added test to ensure we don't synthisize a junk `= 0` when not in `zero initialize` mode

removed member in favor of sharedContext+dictionary.
@@ -1639,7 +1639,8 @@ namespace Slang
}
}

// We must keep zero-initialize in SemanticsDeclHeaderVisitor.
// We must keep zero-initialize for struct fields in
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure I understand the comment here.

@@ -1636,6 +1636,23 @@ namespace Slang
}
}

if (!varDecl->initExpr &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be synthesizing ctor signature in header visitor, and synthesize the ctor body in body visitor, there we call the ctors of the field. Why isn't this possible? We shouln't be checking the init exprs in body visitor IMO.

@@ -498,6 +498,9 @@ struct SharedIRGenContext
Dictionary<Stmt*, IRBlock*> breakLabels;
Dictionary<Stmt*, IRBlock*> continueLabels;

// A reference manager to a ctor's parent
Dictionary<FunctionDeclBase*, IRType*> ctorBaseToParentStruct;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be able to get this with findResultTypeForConstructorDecl?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SemanticsVisitor logic is disjoint from IRGenContext and is not something which can work-without a fully visited SemanticsVisitor?

if (parentStructTypeModifier && decl->members.getCount() == 0)
as<IRStructType>(parentStructTypeModifier->getArg())->defaultCtor = as<IRFunc>(lowered.val);
IRType** maybeParentStruct = this->context->shared->ctorBaseToParentStruct.tryGetValue(decl);
if (maybeParentStruct && decl->members.getCount() == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't think this is needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(reference to previous comment - #3987 (comment))

1. IDefaultInitializer interface was added. If conforming, your type may be zero-initialized. To Conform a `__init()` is required
2. `[OnlyAutoInitIfForced]` was added. This attribute states that a default initializer should only be implicitly called if forced by the compiler (`zero-initialize` for example). This allows types which implicitly/explicitly conform to IDefaultInitialize to have optional auto-init behavior (which is Slang's default for user structs) to be disabled.

* note about `[OnlyAutoInitIfForced]`. This is required for std-lib to not automatically resolve init-expressions for std-lib, but it has the added benifit of allowing user made structs/classes to control the default behavior of initializing
previously defaultConstruct emitting was crashing due to having generics unresolved. By not resolving the default construct immediately, everything works.
@jkwak-work
Copy link
Collaborator

jkwak-work commented May 24, 2024

With this change, if I don't use "-zero-initialize" option, the results are expected to be same as before.
And I tested it locally and saw that one of tests we have shows difference.

tests/language-feature/struct-field-initializers/struct-field-initializer-static.slang

Can you see why the result is changed with your code, and make sure everything works as expected?

The test is not failing but SPIR-V code is different when I compared the outcome with the following command.

slangc -target spirv-asm -o out.spirv tests/language-feature/struct-field-initializers/struct-field-initializer-static.slang

@jkwak-work
Copy link
Collaborator

When I generated GLSL of the test, I see that "data1" is incorrectly unused with your code.
It seems like there is a bug in your implementation.

@ArielG-NV
Copy link
Collaborator Author

ArielG-NV commented May 24, 2024

With this change, if I don't use "-zero-initialize" option, the results are expected to be same as before. And I tested it locally and saw that one of tests we have shows difference.

tests/language-feature/struct-field-initializers/struct-field-initializer-static.slang

Can you see why the result is changed with your code, and make sure everything works as expected?

The test is not failing but SPIR-V code is different when I compared the outcome with the following command.

slangc -target spirv-asm -o out.spirv tests/language-feature/struct-field-initializers/struct-field-initializer-static.slang

Took a bit to check because I wanted to get a clean master first*

There are differences. These of which I can explain, I will share the shaders for ease of reading:

### --> zero-init branch
#version 450
layout(row_major) uniform;
layout(row_major) buffer;

layout(std430, binding = 0) buffer StructuredBuffer_int_t_0 {
    int _data[];
} outputBuffer_0;

struct DefaultStructNoInit_base_0
{
    int data2_0;
};

DefaultStructNoInit_base_0 DefaultStructNoInit_base_x24init_0()
{
    DefaultStructNoInit_base_0 _S1;
    _S1.data2_0 = 2;
    return _S1;
}

struct DefaultStructNoInit_0
{
    DefaultStructNoInit_base_0 base_0;
    int data3_0;
};

DefaultStructNoInit_0 DefaultStructNoInit_x24init_0()
{
    DefaultStructNoInit_0 _S2;
    _S2.base_0 = DefaultStructNoInit_base_x24init_0();
    _S2.data3_0 = 2;
    return _S2;
}

int data1_0;

layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in;
void main()
{
    data1_0 = 2;
    DefaultStructNoInit_0 noInit_0 = DefaultStructNoInit_x24init_0();
    bool _S3;
    if(noInit_0.base_0.data2_0 == 2)
    {
        _S3 = noInit_0.data3_0 == 2;
    }
    else
    {
        _S3 = false;
    }
    outputBuffer_0._data[0U] = int(_S3);
    return;
}

### --> master
#version 450
layout(row_major) uniform;
layout(row_major) buffer;

layout(std430, binding = 0) buffer StructuredBuffer_int_t_0 {
    int _data[];
} outputBuffer_0;

int data1_0;

struct DefaultStructNoInit_base_0
{
    int data2_0;
};

DefaultStructNoInit_base_0 DefaultStructNoInit_base_x24init_0()
{
    data1_0 = 2;
    DefaultStructNoInit_base_0 _S1;
    _S1.data2_0 = 2;
    return _S1;
}

struct DefaultStructNoInit_0
{
    DefaultStructNoInit_base_0 base_0;
    int data3_0;
};

DefaultStructNoInit_0 DefaultStructNoInit_x24init_0()
{
    DefaultStructNoInit_0 _S2;
    DefaultStructNoInit_base_0 _S3 = DefaultStructNoInit_base_x24init_0();
    _S2.base_0 = _S3;
    _S2.data3_0 = 2;
    return _S2;
}

layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in;
void main()
{
    data1_0 = 2;
    DefaultStructNoInit_0 noInit_0 = DefaultStructNoInit_x24init_0();
    bool _S4;
    if(data1_0 == 2)
    {
        _S4 = noInit_0.base_0.data2_0 == 2;
    }
    else
    {
        _S4 = false;
    }
    if(_S4)
    {
        _S4 = noInit_0.data3_0 == 2;
    }
    else
    {
        _S4 = false;
    }

    outputBuffer_0._data[0U] = int(_S4);
    return;
}

@ArielG-NV
Copy link
Collaborator Author

ArielG-NV commented May 24, 2024

Differences;

  1. master: static int data1 = 2; init's in the __init() of a function and at entrypoint; zero-init branch: Now, only entry point sets static variables (correct behavior)

  2. data1 is now placed lower in program code (since referenced first at a later date)

@ArielG-NV
Copy link
Collaborator Author

ArielG-NV commented May 24, 2024

why this happens:
Since we run ensureDecl(struct.member, DeclCheckState::DefaultConstructorReadyForUse), now the code sets up all modifiers+checks+inits correctly before we run bodyVisitor->visitAggTypeDecl().

why this was added to this PR instead of it's own:
It was necessary to fix this to implement zero-initialize.

@ArielG-NV
Copy link
Collaborator Author

ArielG-NV commented May 24, 2024

When I generated GLSL of the test, I see that "data1" is incorrectly unused with your code. It seems like there is a bug in your implementation.

This is correct behavior, master branch does things incorrectly currently.
Notice data_1 is initialized by DefaultStructNoInit_base_x24init_0(), this is wrong, data1 is a static and should only be initialized at the start of main()

@jkwak-work
Copy link
Collaborator

jkwak-work commented May 24, 2024

I think your explanation makes sense.
I was aware that you fixed a bug in this PR.
I didn't know it could be related to an existing test.
Good job.

jkwak-work
jkwak-work previously approved these changes May 25, 2024
csyonghe
csyonghe previously approved these changes Jun 12, 2024
{
auto* invoke = visitor->getASTBuilder()->create<InvokeExpr>();
auto member = visitor->getASTBuilder()->getMemberDeclRef(declRefType->getDeclRef(), defaultCtor);
invoke->functionExpr = visitor->ConstructDeclRefExpr(member, NULL, defaultCtor->loc, nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

NULL->nullptr

`-zero-initialize` test was added to be sure debug pointers are not broken with default init values
@csyonghe csyonghe merged commit b7e8243 into shader-slang:master Jun 12, 2024
2 of 17 checks passed
aroidzap added a commit to aroidzap/slang that referenced this pull request Jun 20, 2024
v2024.1.22

c00f461 remove inline that crashes old glibc version (shader-slang#4398)
fdef653 Improve Direct SPIRV Backend Test Coverage (shader-slang#4396)
33e81a0 [Metal] Fix global constant array emit. (shader-slang#4392)
a38a4fb Make unknown attributes a warning instead of an error. (shader-slang#4391)
a210091 [Metal] Support SV_TargetN. (shader-slang#4390)
2cc9690 Implement for metal `SV_GroupIndex` (shader-slang#4385)
a6b8348 fix the clang/gcc warning (shader-slang#4382)
cfef0c6 Metal: misc fixes and enable more tests. (shader-slang#4374)
2407966 SPIR-V `SV_InstanceId` support in pixel shader (shader-slang#4368)
fba316f Remove `IRHLSLExportDecoration` and `IRKeepAliveDecoration` for non-CUDA/Torch targets (shader-slang#4364)
f0d40ad (origin/master, origin/HEAD, master) capture/replay: implement infrastructure for capture (shader-slang#4372)
ecc6ecb Fix cuda/cpp/metal crash for when using GLSL style shader inputs (shader-slang#4378)
0bf0bf7 Implement Sampler2D for CPP target (shader-slang#4371)
b970b88 Enable full test on macos. (shader-slang#4327)
0574dca Delete glsl_vulkan and glsl_vulkan_one_desc targets. (shader-slang#4361)
085d1a6 Fix emit logic for getElementPtr. (shader-slang#4362)
8813c61 Capability System: Implicit capability upgrade warning/error (shader-slang#4241)
7447fca Add constant folding for % operator. (shader-slang#4359)
8bf7d11 Fix merge error. (shader-slang#4358)
b7e8243 Add slangc flag to `-zero-initialize` all variables (shader-slang#3987)
ccc26c2 Extend the COM-based API to support whole program compilation. (shader-slang#4355)
318adcc Add compiler option to treat enum types as unscoped. (shader-slang#4354)
ec35feb Fix incorrect drop of decoration when translating glsl global var to entrypoint param. (shader-slang#4353)
c194af8 Fix crash on invalid entrypoint varying parameter. (shader-slang#4349)
7a4757d Implicit register binding for hlsl to non-hlsl targets (shader-slang#4338)
180d6b1 Fix duplicate SPIRV decorations. (shader-slang#4346)
fa8c11e Add option to preserve shader parameter declaration in output SPIRV. (shader-slang#4344)
3fe4a77 Fix crash when using optional type in a generic. (shader-slang#4341)
5da06d4 Fix global value inlining for spirv_asm blocks. (shader-slang#4339)
7e79669 [gfx] Metal improvements (shader-slang#4337)
6909d65 SPIRV backend: add support for tessellation stages, (shader-slang#4336)
ef20d93 Test more texture types in Metal (shader-slang#4333)
5a28968 [gfx] Metal texture fixes (shader-slang#4331)
df0a201 Support integer typed textures for GLSL (shader-slang#4329)
51d3585 Remove duplicate `VkPhysicalDeviceComputeShaderDerivativesFeaturesNV` extension structure in vk-api.h (shader-slang#4335)
6d5ef9b Fix `GetAttributeAtVertex` for spirv and glsl targets. (shader-slang#4334)
21bbebb Address glslang ordering requirments for 'derivative_group_*NV' (shader-slang#4323)
72016f9 Partial implementation of static_assert (shader-slang#4294)
712ce65 enable more metal tests (shader-slang#4326)
38c0bac Fix SPIRV emit for `Flat` decoration and TessLevel builtin. (shader-slang#4318)
b5cdd83 Support all integer typed indices in StructuredBuffer Load/Store/[]. (shader-slang#4311)
6857dd5 [gfx] Metal graphics support (shader-slang#4324)
0974463 Fix typos in the docs (shader-slang#4322)
9a23a9a SPIRV `Block` decoration fixes. (shader-slang#4303)
bc680e7 Add initial draft auto-diff basics and IR overview documents (shader-slang#4216)
ee812d1 Disallow certain types of decls in `interface` to provide better diagnostic message. (shader-slang#4312)
65928af Metal system value overhaul. (shader-slang#4308)
e39ceab Adding functional test for GLSL texture functions (shader-slang#4306)
2a45bc3 Support HLSL `.mips` syntax. (shader-slang#4310)
056a4b9 Small SPIRV emit cleanup around vector element extract. (shader-slang#4309)
f83fe55 Make CTS failure report more obvious (shader-slang#4302)
78d34f3 Improve documentation and example formatting consistency (shader-slang#4299)
7c6faf6 Precompute UIntSet from individual capabilities inside generator (shader-slang#4269)
004fe27 Metal compute tests (shader-slang#4292)
72f10a8 Fixed profile string per request in pr#4268 (shader-slang#4297)
a709e02 Update capability-generator-main.cpp (shader-slang#4295)
d301267 Typo in 06-interfaces-generics.md (shader-slang#4284)
fa664d1 Fix build warnings and treat warnings as error on CI (shader-slang#4276)
f149052 Remove unnecessary call to __requireComputeDerivative (shader-slang#4283)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to zero-initialize all variables
3 participants