Fix DxilPayloadFieldAnnotation::GetPayloadFieldQualifier

[jenatali]

Fixes #6941

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 75ff50caa046a054747ae15b5c1910a4c8aa1917 073028caa634222e36f44ad6c36ef859b39c8327 -- lib/DXIL/DxilTypeSystem.cpp tools/clang/unittests/HLSL/DxilModuleTest.cpp

View the diff from clang-format here.

diff --git a/tools/clang/unittests/HLSL/DxilModuleTest.cpp b/tools/clang/unittests/HLSL/DxilModuleTest.cpp
index 85b412a2..98931278 100644
--- a/tools/clang/unittests/HLSL/DxilModuleTest.cpp
+++ b/tools/clang/unittests/HLSL/DxilModuleTest.cpp
@@ -574,7 +574,8 @@ TEST_F(DxilModuleTest, PayloadQualifier) {
                   "  int b : read(caller) : write(miss);\n"
                   "};\n\n"
                   "[shader(\"miss\")]\n"
-                  "void Miss( inout Payload payload ) { payload.a = 4.2; payload.b = 1; }\n";
+                  "void Miss( inout Payload payload ) { payload.a = 4.2; "
+                  "payload.b = 1; }\n";
 
   c.Compile(shader, L"lib_6_6", arguments, {});
 

• Check this box to apply formatting changes to this branch.

[damyanp]

LGTM - I can't say I fully understand exactly why the simplified code is equivalent. Have you confirmed that there's any existing tests at all that are validating that it at least sometimes did what it was supposed to do?

[jenatali]

The simplified code is not equivalent, this isn't just a simplification, it's also a bugfix.

According to the DXR spec:
<none> means "The payload field is neither available for reading nor are its contents modified by the given stage."

That would map to the NoAccess value. The existing test for this metadata helper only had one payload field, which had every stage represented as either read, write, or read-write. There was no stage which was completely omitted from the annotations. By adding the second payload field which omits closesthit/anyhit, I can verify that the old code incorrectly returned ReadWrite for those stages.

The old code was correct as long as the stage had either a read or write bit set, or both, it was only incorrect in that it returned ReadWrite instead of NoAccess. It was just... very verbose and therefore confusing in the way that it calculated the return value.

[damyanp]

Oops, I misspoke by "equivalent"! The nuance I was trying to get at was that presumably there are some cases where the new code would be expected to return the same results as before, and that there is some test coverage that continues to validate that.

[jenatali]

Yeah, the modified test had one field which verified the read, read-write, and write behaviors. Those cases continue to work.

The old code took the 2 bits for read/write and constructed:

• If the write bit is set:
  • If the read bit is set, return read-write
  • Else return write
• Else if the read bit is set, return read
• Else return read-write

I could have simply changed the "default" temporary value to NoAccess instead of ReadWrite, but also the code was much more complicated than it needed to be, because NoAccess is 0, and ReadWrite is 3, so the bitwise & just gives us the right answer.

[tex3d]

Looks good.