[NNNN] Document local resource behavior

[bob80905]

This PR lays out a plan for test-driven documentation of local resource behavior.
Addresses #180

Assisted by: Github Copilot

[inbelic]

I would have expected these to be an error in clang as well. Although not at the semahlsl level if that is what is being checked to determine whether it is a warning/error.

[bob80905]

Yep, but IIRC, this will be a key difference between clang and DXC, so I believe the intent is for clang not to reject this.

[inbelic]

The intent is that clang should reject it, just not during sema, this is because it is not easy to determine whether or not the used resource is unique at that AST level. Instead we generate a warning and then error later during lowering to dxil.

There is a potential future where we do support passing handles to resources around, but that isn't something that is planned.

[inbelic]

I guess one thing here, has there been a check to what test cases are already covered?

[bob80905]

No there hasn't, though I think it would be somewhat reasonable for there to be duplicates.
In the same code, we could, for example, test the AST in SemaHLSL to ensure that resource AST nodes are properly present, while the same HLSL can be used to verify local resource behavior. I know @hekota has some similar looking tests to the ones proposed, though the intent of the test is completely different from this.

[inbelic]

Imo we definitely shouldn't have exact duplicate tests. If they use the same source but are testing different things that is okay, they aren't duplicates. Ideally I'd think it might be nice to combine those cases in one file with multiple run lines (if the directory makes sense for both). There is a cost to having redundant tests.

[bogner]

I find these tables kind of unwieldy - they're very hard to read in the markdown source and are pretty noisy when rendered as well. We're also missing a crucial piece of information here, which is "what is the expected behaviour?", and it isn't clear if "Clean" is actually what we want in a lot of these cases.

What if we formatted these something like so, where we say what's expected and use a checkmark to indicate that the compiler matches the expectation (some useful examples cherry picked):

---

Alias from global (buf = gBuf0)

• Expected: Allowed, creates a local variable that references the global
  resource.
• DXC: ✅
• Clang: ✅

Self-assignment of uninitialized resource (Out = Out)

• Expected: Error (use of uninitialized resource)
• DXC: ✅ Error (codegen): local resource not guaranteed to map to unique global resource
• Clang: ❌ Compiles cleanly - statement is ignored

Default-init store (unbound handle)

• Expected: Error for unbound resource use
• DXC: ✅ Error (codegen): local resource not guaranteed to map to unique global resource
• Clang: ❌ Crash in codegen (DXIL Op Lowering assertion on unbound handle)

Comma expression init ((gBuf0, gBuf1))

• Expected: Allowed, value is last value of the comma expression
• DXC: ✅
• Clang: ✅ - note: warns left operand of comma operator has no effect

---

[bob80905]

I personally find the tables readable, though depending our resolution to the point below I would be willing to adjust the table format to the format you've written. However, I disagree at this point with including expected behavior. This will open up a huge can of worms on what the compiler ought to do and ought not to do, for 70+ tests, and will likely be very unproductive. We absolutely should intend to clearly delineate compiler behavior for each and every test proposed here, but the purpose of this PR is to document what each compiler does, and possibly in some cases, allow that documentation to drive our decisions on what our expectations ought to be.

In short, I think it is alright in this stage to not make a claim on expected compiler behavior, and to directly document behavior. Future work can take care of making ought-type decisions, but this proposal is strictly about documenting local resource behavior.
For further context, see the comment @inbelic made above:

[bogner]

We want an error here, right?

[bob80905]

I would think that we would want clang to emit an error here, yes. Though at this point, I don't believe that question is within the scope of this proposal.

[bogner]

I'm surprised that DXC is "clean" here - shouldn't this be an error? There's no way for this to do something sensible.

[bob80905]

My opinion is that this ought to be an error, though as I commented above, I think this is out of the scope of the proposal.

[bogner]

This doesn't make sense to me - tests should only be in the offload test suite if there is an expected valid/correct behaviour. This may be different than at least one compiler succeeding at compiling something, as there may be cases where the compiler does an obviously wrong thing but succeeds at compiling, and there is no sensible "right" thing to do.

[bob80905]

Suppose there exists a shader that ought to be compiled into a clean output, but it has undefined runtime behavior.
Does this shader exist? Either the compiler ought to emit an error about undefined runtime behavior, in which case it wouldn't be placed in the offload test suite, or the compiler ought to compile cleanly and we'd get different runtime behavior based on IHV.
Suppose 2 IHV drivers, A and B, run the shader, and A crashes, while B runs fine.
I would think in this case, we'd effectively utilize the offload test suite to catch the fact that A crashes, and maybe file a driver bug on A?
I am not quite sure what we'd do in this case, but I would think there would be value in having added it to the offload test suite, to identify that peculiarity in behavior.

But the other option you seem to be suggesting: we keep the test in the llvm repo (so we can't test compilation on DXC, unless we add a copy of the test to DXC, which we probably want to avoid). And we just test that the shader compiles.
The added difficulty here is that we will need to make a judgement call for each test on whether the test's use case is valid / correct. This is something I want to avoid in this proposal if possible.

Given these concerns, does adding these tests to the offload test suite make sense?

[bogner]

Subdirectories per compiler doesn't sound right - shouldn't we simply XFAIL the tests for the compiler that doesn't succeed? We should be filing an issue for any of these tests that is indicating a bug, in any case.

[bob80905]

While normally we would XFAIL tests that fail on specific compilers and file a bug, in this specific context I think it is not always the case that we would want to do this.
Some shaders may exist that ought to behave one way and compile cleanly on DXC, but should fail on clang.
We wouldn't want to associate a bug on clang with this shader.
Same thing in the inverse case.
So, my reasoning behind this was, what if we organized the tests so that we could quickly identify / locate HLSL local resource use cases that are compiler-specific?

It would be nice to say "I want to know all the local resource use cases that are specific to DXC and pass on the DXC compiler, but the clang compiler rejects it."
These subdirectories allow for this organization, and prevent us from needing to add an associated bug.
I think this class of tests are somewhat different than the other tests, since some are actually not expected to behave identically across compilers.
So the idea is, tests that do behave identically (note, not ought, but do), are placed together, for organization, and tests that behave differently are categorized.

What do you think?

My vision here was that this set of tests initially documents test behavior only, but over time, transforms into "ought", what we want and expect the tests to behave like. So I think there is value in isolating tests where they ought / we want them to behave differently based on compiler.

[bogner]

I think we need the categories to be more like:

• clang/test/SemaHLSL/Resources/Local-Resources/ for tests that emit warnings and errors in clang's Sema
• llvm/test/CodeGen/DirectX/ for errors that can only be resolved late (some of the ambiguous resource cases come to mind) and thus can't be handled in Sema. This includes cases where Sema warns but the backend has a hard error.
• offload-test-suite/test/Feature/LocalResources/ for offload tests of correct behaviour
• Probably some tests for correct codegen in clang/test/CodeGenHLSL and llvm/test/CodeGen/DirectX as we implement and fix bugs

[bob80905]

My comment in response to your "Subdirectories per compiler " comment above addresses this comment.

[bogner]

I disagree at this point with including expected behavior. This will open up a huge can of worms on what the compiler ought to do and ought not to do, for 70+ tests, and will likely be very unproductive. We absolutely should intend to clearly delineate compiler behavior for each and every test proposed here, but the purpose of this PR is to document what each compiler does, and possibly in some cases, allow that documentation to drive our decisions on what our expectations ought to be.

I understand your concern about potentially having to figure out expectations for every single case here in order to make progress, but I don't think pretending that we have no expectations whatsoever is doing us any good. Functionally, this PR is saying that our expectations are that both compilers "compile cleanly" for every thing listed here, but I don't really see how that's a useful metric. It also makes it harder to talk about what the compilers are actually doing - if it compiles cleanly but generates garbage code it's a different thing than if it compiles the operation successfully, and if we don't differentiate in this document we basically have to do the entire investigation again for each operation as we go.

I think a lot of the cases here have very obvious expected behaviour, and at the very least we should be listing those and comparing the compiler's behaviour to that. This tells us that we need a test, and easily points out whether or not we're going to need to file issues and XFAIL that test. This makes it much easier to differentiate between "this is valid HLSL" and "the compiler should emit an error here" in ways that still give us the space to recognize that a compiler crashing doesn't conform to "the compiler should emit an error".

However, we should also consider that "the behaviour is undefined", or "we don't have consensus or a clear picture what the behaviour should be" are valid. This would also make it clear where we need to file issues on the HLSL spec work to go and do the work to decide what we want to do, including in cases where we want to deliberately differ between Clang and DXC.

In short, I think it is alright in this stage to not make a claim on expected compiler behavior, and to directly document behavior. Future work can take care of making ought-type decisions, but this proposal is strictly about documenting local resource behavior.

If this is all we do here, the only reasonable next step is to immediately do the work to make the claims on expected compiler behaviour. I do not believe this is sufficient for us to decide which tests we need, and so we'll end up needing to do as I suggest above anyway, but instead of it being done in a comprehensive survey the conversation will be scattered among the PRs that add the tests themselves.

Suppose there exists a shader that ought to be compiled into a clean output, but it has undefined runtime behavior.
Does this shader exist? Either the compiler ought to emit an error about undefined runtime behavior, in which case it wouldn't be placed in the offload test suite, or the compiler ought to compile cleanly and we'd get different runtime behavior based on IHV.
Suppose 2 IHV drivers, A and B, run the shader, and A crashes, while B runs fine.
I would think in this case, we'd effectively utilize the offload test suite to catch the fact that A crashes, and maybe file a driver bug on A?
I am not quite sure what we'd do in this case, but I would think there would be value in having added it to the offload test suite, to identify that peculiarity in behavior.

In this case we would be identifying an HLSL spec issue. There are essentially three options:

• The spec defines the behaviour, in which case there's a correct result and any driver that doesn't have that result is incorrect.
• The spec disallows the behaviour, in which case the compilers should reject the shader and emit an error.
• The spec classifies the behaviour as undefined or implementation defined. This should generally be avoided, but in this case we can't write a meaningful runtime test.

Some shaders may exist that ought to behave one way and compile cleanly on DXC, but should fail on clang.
We wouldn't want to associate a bug on clang with this shader.
Same thing in the inverse case.

I largely disagree. This is why documenting our expectations is important. Neither DXC nor Clang are the arbiter for what's correct, the HLSL spec is. For both HLSL 202X and 202Y, we should have clear definitions of what is correct and what isn't. If Clang or DXC disagrees with those definitions, then that's a bug in Clang or DXC (whether we intend to fix it or not). If those definitions are unclear or missing, that's an issue on the spec itself, and needs to be fixed. For cases where the behaviour in 202X differs from HLSL 2021 or earlier, or where 202Y differs from 202X, any tests need to specify the language version, and can only be meaningfully run on compilers that support that language version.