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

Warnings for uninitialized values #4530

Merged

Conversation

venkataram-nv
Copy link
Collaborator

@venkataram-nv venkataram-nv commented Jul 2, 2024

Addresses #3832

Changes:

  • Previously slang-ir-uninitialized-out-params.* dictated diagnostics logic for output parameters. Since this has similar behavior to checking usage of uninitialized variables, it has been refactored into slang-ir-uninitialized-values.* which handles both.
  • Many std functions are meta function which operate on the type of the parameter. However, in doing so, they instantiate an undefined variable of the desired type and then pass that to some compiler intrinsic. These intrinsic have their own op codes so they can be detected and ignored.
  • Likewise, differentiable functions (and functions synthesized in the process) are ignored, since requiring certain values to be initialized may conflict with the autodiff pass.

What this does not implement (could be future PRs):

  • Checking uninitialized fields within constructors
  • Partially uninitialized values with respect to data structure (e.g. arrays/structs/vector types)
  • Partially uninitialized values with respect to control flow (e.g. if/else/loop)

@venkataram-nv venkataram-nv added pr: non-breaking PRs without breaking changes pr: breaking change PRs with breaking changes and removed pr: non-breaking PRs without breaking changes labels Jul 2, 2024
@venkataram-nv venkataram-nv self-assigned this Jul 2, 2024
@venkataram-nv venkataram-nv force-pushed the warn-uninitialized-values branch 2 times, most recently from a6e2550 to 76c1dcf Compare July 3, 2024 01:10
@venkataram-nv venkataram-nv marked this pull request as ready for review July 3, 2024 20:42
@venkataram-nv venkataram-nv force-pushed the warn-uninitialized-values branch from e2a46a8 to a399f9f Compare July 4, 2024 00:25
source/slang/slang-ir-use-uninitialized-values.cpp Outdated Show resolved Hide resolved
source/slang/slang-ir-use-uninitialized-values.cpp Outdated Show resolved Hide resolved
source/slang/slang-ir-use-uninitialized-values.cpp Outdated Show resolved Hide resolved
source/slang/slang-ir-use-uninitialized-values.cpp Outdated Show resolved Hide resolved
source/slang/slang-ir-use-uninitialized-values.cpp Outdated Show resolved Hide resolved
source/slang/slang-ir-use-uninitialized-values.cpp Outdated Show resolved Hide resolved
tests/bugs/gh-1990.slang Outdated Show resolved Hide resolved
@venkataram-nv venkataram-nv force-pushed the warn-uninitialized-values branch 3 times, most recently from af0fff0 to 5264ca5 Compare July 5, 2024 22:47
@ArielG-NV
Copy link
Contributor

ArielG-NV commented Jul 8, 2024

This PR seems to not handle the case of groupshared memory (as tested).
This memory is not zero initialized from what I remember, it is undefined behavior to not initialize this memory before use.

For this case there are 3 scenarios:

  1. For-sure Initialized (possible through store and atomicStore/InterlockedExchange operations)
  2. For-sure uninitialized
  3. Maybe (un)initialized -- thread_0 may initialize groupshared memory for thread_1 under some branch which runs only on some threads (likely not easily detectable).

@venkataram-nv
Copy link
Collaborator Author

venkataram-nv commented Jul 8, 2024

@ArielG-NV
I see, I will add that part then. Should we be emitting similar diagnostics for global variables (e.g. static float x;)?

@ArielG-NV
Copy link
Contributor

ArielG-NV commented Jul 8, 2024

I see, I will add that part then. Should we be emitting similar diagnostics for global variables (e.g. static float x;)?

I don't know the answer to this question since the behavior of static/equivalent across platforms is inconsistent:

  1. Metal: We do legalization to handle this case, the value is effectively uninitialized
  2. HLSL: zero initializes, "[if a static] does not include an initializer, the value is set to zero"
  3. GLSL: uninitialized, "Global variables without storage qualifiers that are not initialized in their declaration or by the application will not be initialized, but rather will enter main() with undefined values"
  4. SPIR-V: uninitialized, spec does not say anything about this case but glslang (GLSL->SPIR-V) leaves globalVar's as uninitialized.

@venkataram-nv venkataram-nv force-pushed the warn-uninitialized-values branch from 38fa750 to cd39378 Compare July 9, 2024 17:21
@venkataram-nv venkataram-nv requested a review from ArielG-NV July 9, 2024 17:48
@venkataram-nv
Copy link
Collaborator Author

Currently the warning system does not properly handle constructors (this is the reason for the modification in ctor-ordinary-retval-legal.slang). I intend to fix this in another PR since fixing this involves modifying some other files.

Comment on lines 148 to 158
auto users = getConcernableUsers(inst);

addresses.add(inst);
for (auto user : users)
{
if (isAliasable(user))
{
auto instAddresses = getAliasableInstructions(user);
addresses.addRange(instAddresses);
}
}
Copy link
Contributor

@ArielG-NV ArielG-NV Jul 9, 2024

Choose a reason for hiding this comment

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

Question:
do we need to allocate a list here- would the following work?

Suggested change
auto users = getConcernableUsers(inst);
addresses.add(inst);
for (auto user : users)
{
if (isAliasable(user))
{
auto instAddresses = getAliasableInstructions(user);
addresses.addRange(instAddresses);
}
}
addresses.add(inst);
for (auto use = inst->firstUse; use; use = use->nextUse)
{
IRInst* user = use->getUser();
// Meta instructions only use the argument type
if (isMetaOp(user) || !isAliasable(user))
continue;
addresses.addRange(getAliasableInstructions(user));
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that works, thanks for the suggestion.

@venkataram-nv venkataram-nv requested a review from ArielG-NV July 9, 2024 19:44
@ArielG-NV
Copy link
Contributor

I was testing and it seems the following code does not warn despite using uninitialized values.

Example:

RWStructuredBuffer<int> output;

void inoutTest(inout int badInt)
{
    output[0] = badInt;
}

void inTest(int badInt)
{
    output[1] = badInt;
}

[numthreads(1, 1, 1)]
void computeMain(uint3 dispatchThreadID : SV_DispatchThreadID)
{
    int input1;
    inoutTest(input1);

    int input2;
    inTest(input2);

    output[2] = input1;
    output[3] = input2;
}
  1. inoutTest(input1);-> output[0] = badInt; should warn
  2. inTest(input2);-> output[1] = badInt; should warn
  3. output[2] = input1; should warn
  4. output[3] = input2; should warn

@venkataram-nv
Copy link
Collaborator Author

@ArielG-NV
Currently the methods do not analyze the behavior of functions with respect to its input (e.g. if its passed as an out parameter). I plan to separate that into a separate smaller issue (along with another for constructors). As for buffer accesses, the assumption is that it is a parameter that is passed into the shader, so usage of it is not treated as undefined.

@ArielG-NV
Copy link
Contributor

ArielG-NV commented Jul 9, 2024

Currently the methods do not analyze the behavior of functions with respect to its input

👍Sounds good. Likely worth making an issue with all of the next steps.

Otherwise, code looks good to me, I will let Yong review before approval.

csyonghe
csyonghe previously approved these changes Jul 9, 2024
source/slang/slang-ir-use-uninitialized-values.cpp Outdated Show resolved Hide resolved
@venkataram-nv venkataram-nv merged commit 0e6c5c5 into shader-slang:master Jul 9, 2024
17 checks passed
@venkataram-nv venkataram-nv deleted the warn-uninitialized-values branch July 9, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: breaking change PRs with breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants