[SER] Diagnose HitObject in unsupported declaration contexts #7376

simoll · 2025-04-24T13:52:31Z

Generalize long vector diagnostics code to HitObjects.
Diagnose unsupported use of HitObject in globals, entry params/returns and various other shader-kind-specific contexts.
Create HitObject variants from the invalid-longvec-decls*.hlsl tests to make sure all cases are covered.

Specification: https://github.com/microsoft/hlsl-specs/blob/main/proposals/0027-shader-execution-reordering.md

Closes #7234 [SER] Diagnose and validate illegal use of HitObject in unsupported contexts (discussed offline)

- Generalize long vector diagnostics code to HitObjects. - Diagnose unsupported use of HitObject in globals, entry params/returns and various other shader-kind-specific contexts. - Create HitObject variants from the invalid-longvec-decls*.hlsl tests to make sure all cases are covered. Specification: https://github.com/microsoft/hlsl-specs/blob/main/proposals/0027-shader-execution-reordering.md Bug: microsoft#7234 [SER] Diagnose and validate illegal use of HitObject in unsupported contexts

tex3d

Mainly, I don't think we should be adding as much special-case code for HitObject, where the code should be the same for other object types, such as detecting whether we have an intangible HLSL object type inside another type decl. HitObject should be considered one of a variety of intangible HLSL object types that are handled uniformly in a bunch of contexts.

But also, combining the HitObject diagnostic with the long vector one is a bit confusing, as the contexts that disallow HitObject, and other intangible objects generally, are different than the contexts that disallow long vectors. This is conceptually a different rule.

tools/clang/include/clang/Basic/DiagnosticSemaKinds.td

tools/clang/lib/AST/DeclCXX.cpp

tools/clang/lib/AST/HlslTypes.cpp

and err_hlsl_objectintemplateargument

simoll · 2025-05-02T14:26:20Z

This is a major update. Changes:

Created a new err_hlsl_unsupported_object_context diagnostic. %select values are compatible with the err_hlsl_unsupported_long_vector one plus new entries for other contexts.
Folded err_hlsl_node_record_object and err_hlsl_objectintemplateargument into that new diagnostic.
Split off object diagnostic from long vector diagnostic again.
Created a one-stop diagnostic infra to tell whether an HLSL object is allowed in a certain context or not (eg see AllowObjectInContext).
Added HitObject test case for groupshared, static global variables (no) and regular user functions (only the last is allowed)
Using the infra for HitObject but this could be used for other object types in unsupported contexts (RayQuery, ..)

tools/clang/include/clang/Basic/DiagnosticSemaKinds.td

pow2clk

So that's a lot of nitpicks from me. @tex3d still needs to reset his feedback for this to go in. As far as I can tell, the feedback he gave has mostly been responded to, but could further be addressed by merging the loose ContainsLongVector checks into DiagnoseTypeElements and its implementor functions, which I would also prefer.

tools/clang/include/clang/Basic/DiagnosticSemaKinds.td

tools/clang/lib/Sema/SemaHLSL.cpp

tools/clang/lib/Sema/SemaHLSLDiagnoseTU.cpp

tools/clang/test/SemaHLSL/hlsl/objects/HitObject/hitobject_traceinvoke_payload_udt.hlsl

tools/clang/test/SemaHLSL/hlsl/types/invalid-hitobject-decls-struct.hlsl

pow2clk · 2025-05-12T20:40:37Z

A large chunk of my nitpicks become moot if ContainsLongVec is merged as the TODO comment suggests.

Co-authored-by: Greg Roth <[email protected]>

…truct.hlsl Co-authored-by: Greg Roth <[email protected]>

Co-authored-by: Greg Roth <[email protected]>

types need to be checked

github-actions · 2025-05-13T05:18:55Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Co-authored-by: Greg Roth <[email protected]>

…ctor

Co-authored-by: Greg Roth <[email protected]>

type instead)

simoll · 2025-05-13T08:45:07Z

So that's a lot of nitpicks from me. @tex3d still needs to reset his feedback for this to go in. As far as I can tell, the feedback he gave has mostly been responded to, but could further be addressed by merging the loose ContainsLongVector checks into DiagnoseTypeElements and its implementor functions, which I would also prefer.

Thanks for the review. I've folded the CheckLongVector logic into DiagnoseTypeElements. This removed a lot of redundant code and the HLSLLongVector tracking bit as this is all handled by this one diagnostic function now.

pow2clk

Looks good! I have just a couple of nits you might consider if you end up making another iteration of this. I'm not sure why it says a merge resolution is required. When I tried to inspect the conflict, it showed zero conflicts.

pow2clk · 2025-05-15T02:34:48Z

tools/clang/include/clang/AST/DeclCXX.h


-    /// \brief Whether this class contains at least one member or base
-    ///  class containing an HLSL vector longer than 4 elements.
-    bool HasHLSLLongVector : 1;


I confess I didn't anticpate that merging the longvector check with the other type checks would result in removing this bit, but I see how it follows and I don't object.

pow2clk · 2025-05-15T02:36:48Z

tools/clang/lib/Sema/SemaHLSL.cpp

+      const TypeDiagContext DiagContext = TypeDiagContext::TessellationPatches;
      if (DiagnoseTypeElements(*m_sema, argLoc.getLocation(), arg.getAsType(),
-                               TypeDiagContext::TessellationPatches))
+                               DiagContext, DiagContext))


As frequently as the pattern of both objects and long vectors having the same context occurs, it may make sense to have an overload of DiagnoseTypeElements that takes a single context that is propagated to both for the longer overload, but I won't insist on it if there are no other commits to be made to this PR that change might be packaged with.

pow2clk · 2025-05-15T02:38:09Z

tools/clang/lib/Sema/SemaHLSL.cpp

  case AR_TOBJ_OBJECT:
    Empty = false;
-    if (AllowObjectInContext(Ty, DiagContext))
+    if (!CheckObjects || AllowObjectInContext(Ty, ObjDiagContext))


I didn't expect and after review didn't observe any cases where the call to this would result in CheckObjects being false. There's nothing wrong with handling the condition in the interest of making it more robust, but I wanted to confirm I had that right.

pow2clk · 2025-05-15T02:41:21Z

tools/clang/lib/Sema/SemaHLSL.cpp

+
+    // Disallow long vecs from $Global cbuffers.
+    if (!isStatic && !isGroupShared && !IS_BASIC_OBJECT(basicKind))
+      LongVecDiagContext = TypeDiagContext::CBuffersOrTBuffers;


I was imagining else if (!IS_BASIC_OBJECT(basicKind)) as the final else if after the else if (isStatic) on line 15378 which would result in something more concise, but I won't insist. If you make another change to the PR, you might consider that consolidation.

tools/clang/lib/Sema/SemaHLSL.cpp

This reverts commit 1b8af76.

- Recurse bases in IsHLSLCopyableAnnotatableRecord - Don't skip DiagnoseTypeElements for templated Load/Store - Don't skip existing Payload diagnostics when DiagnoseTypeElements fails - Update tests, including moving fields pointed to by notes to beginning to avoid shifting fixed location references when modifying test cases

pow2clk

Looks good! Better diagnostics overall!

damyanp

Approving based on previous approvals before the most recent change.

github-project-automation bot added this to HLSL Roadmap Apr 24, 2025

github-project-automation bot moved this to New in HLSL Roadmap Apr 24, 2025

simoll mentioned this pull request Apr 24, 2025

SER implementation tracker #7214

Closed

41 tasks

tex3d added this to HLSL Support Apr 24, 2025

tex3d self-assigned this Apr 24, 2025

tex3d requested changes Apr 24, 2025

View reviewed changes

tools/clang/include/clang/Basic/DiagnosticSemaKinds.td Outdated Show resolved Hide resolved

tools/clang/lib/AST/DeclCXX.cpp Outdated Show resolved Hide resolved

tools/clang/lib/AST/HlslTypes.cpp Outdated Show resolved Hide resolved

github-project-automation bot moved this from New to In progress in HLSL Roadmap Apr 24, 2025

damyanp moved this to Active in HLSL Support Apr 25, 2025

simoll added 2 commits May 2, 2025 10:30

Merge remote-tracking branch 'msft/main' into NEWBASE

18e7c1c

Split-off from LongVector diagnostics / Fold err_hlsl_node_record_object

57ff5a3

and err_hlsl_objectintemplateargument

simoll changed the title ~~[SER] Generalize long vector diagnostics to HitObject~~ [SER] Diagnose HitObject in unsupported declaration contexts May 2, 2025

simoll requested a review from tex3d May 2, 2025 16:07

tex3d reviewed May 9, 2025

View reviewed changes

tools/clang/include/clang/Basic/DiagnosticSemaKinds.td Outdated Show resolved Hide resolved

tools/clang/include/clang/Basic/DiagnosticSemaKinds.td Outdated Show resolved Hide resolved

simoll added 2 commits May 9, 2025 13:16

Remove redundant '' around %0 in unsupported_object_context diag msg

2c128ef

'type parameters' -> 'builtin template parameters'

e39fe2f

damyanp assigned pow2clk May 12, 2025

pow2clk reviewed May 12, 2025

View reviewed changes

simoll and others added 10 commits May 13, 2025 06:24

Update tools/clang/include/clang/Basic/DiagnosticSemaKinds.td

92bb7fc

Co-authored-by: Greg Roth <[email protected]>

Update tools/clang/lib/Sema/SemaHLSL.cpp

6bb2dbf

Co-authored-by: Greg Roth <[email protected]>

Update tools/clang/lib/Sema/SemaHLSL.cpp

e0e063a

Co-authored-by: Greg Roth <[email protected]>

Update tools/clang/lib/Sema/SemaHLSL.cpp

c039f8c

Co-authored-by: Greg Roth <[email protected]>

Update tools/clang/lib/Sema/SemaHLSLDiagnoseTU.cpp

2a7b39c

Co-authored-by: Greg Roth <[email protected]>

Update tools/clang/lib/Sema/SemaHLSL.cpp

21447bc

Co-authored-by: Greg Roth <[email protected]>

Update tools/clang/lib/Sema/SemaHLSL.cpp

2be84a2

Co-authored-by: Greg Roth <[email protected]>

Update tools/clang/lib/Sema/SemaHLSL.cpp

85746fb

Co-authored-by: Greg Roth <[email protected]>

Update tools/clang/test/SemaHLSL/hlsl/types/invalid-hitobject-decls-s…

36d678c

…truct.hlsl Co-authored-by: Greg Roth <[email protected]>

Update tools/clang/lib/Sema/SemaHLSL.cpp

96c6b8c

Co-authored-by: Greg Roth <[email protected]>

simoll added 2 commits May 13, 2025 06:50

static_cast<unsigned>(TypeDiagContext)

c07e548

Default case: accept as non-Empty and add comment that only recursive

004dc1d

types need to be checked

simoll and others added 9 commits May 13, 2025 07:20

Update tools/clang/lib/Sema/SemaHLSL.cpp

244c59c

Co-authored-by: Greg Roth <[email protected]>

function description

fa895f7

Clang format

9db0556

Assert TypeDiagContext enum value compatible with unsupported_long_ve…

e7f8c5a

…ctor

Update tools/clang/lib/Sema/SemaHLSL.cpp

fea88fb

Co-authored-by: Greg Roth <[email protected]>

Fold isGroupShared in with isStatic/isGlobal diag case

701c680

Clang format

001eb50

Fold unsupported_long_vec into DiagnoseElementTypes

ae98e48

Remove HLSLLongVector tracking bit (DiagnoseElementTypes descends into

2fd825f

type instead)

simoll requested review from pow2clk and tex3d May 13, 2025 09:06

pow2clk approved these changes May 15, 2025

View reviewed changes

tex3d approved these changes May 15, 2025

View reviewed changes

simoll added 2 commits May 15, 2025 09:20

Merge remote-tracking branch 'msft/main' into ser_diaghitobject_patch

43449f4

Update test after merge

1b8af76

tex3d reviewed May 15, 2025

View reviewed changes

tools/clang/lib/Sema/SemaHLSL.cpp Outdated Show resolved Hide resolved

tex3d added 2 commits May 15, 2025 12:06

Revert "Update test after merge"

36e9615

This reverts commit 1b8af76.

pow2clk approved these changes May 15, 2025

View reviewed changes

Diagnose long vector in payload call argument case

58b7642

damyanp approved these changes May 16, 2025

View reviewed changes

tex3d merged commit f9c2d5d into microsoft:main May 16, 2025
12 checks passed

github-project-automation bot moved this from Active to Closed in HLSL Support May 16, 2025

github-project-automation bot moved this from In progress to Done in HLSL Roadmap May 16, 2025

simoll deleted the ser_diaghitobject_patch branch May 19, 2025 04:37

damyanp removed this from HLSL Support Jun 25, 2025

[SER] Diagnose HitObject in unsupported declaration contexts #7376

[SER] Diagnose HitObject in unsupported declaration contexts #7376

Uh oh!

Conversation

simoll commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tex3d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simoll commented May 2, 2025

Uh oh!

Uh oh!

Uh oh!

pow2clk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pow2clk commented May 12, 2025

Uh oh!

github-actions bot commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simoll commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pow2clk left a comment

Choose a reason for hiding this comment

Uh oh!

pow2clk May 15, 2025

Choose a reason for hiding this comment

Uh oh!

pow2clk May 15, 2025

Choose a reason for hiding this comment

Uh oh!

pow2clk May 15, 2025

Choose a reason for hiding this comment

Uh oh!

pow2clk May 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pow2clk left a comment

Choose a reason for hiding this comment

Uh oh!

damyanp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

simoll commented Apr 24, 2025 •

edited

Loading

github-actions bot commented May 13, 2025 •

edited

Loading

simoll commented May 13, 2025 •

edited

Loading