This is an archive of the discontinued LLVM Phabricator instance.

[HLSL] Pointers are unsupported in HLSL
ClosedPublic

Authored by beanz on Apr 5 2022, 5:28 PM.

Details

Summary

HLSL does not support pointers or references. This change generates errors in sema for generating pointer, and reference types as well as common operators (address-of, dereference, arrow), which are used with pointers and are unsupported in HLSL.

Diff Detail

Event Timeline

beanz created this revision.Apr 5 2022, 5:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 5:28 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
beanz requested review of this revision.Apr 5 2022, 5:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 5:28 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I think you're going to need semantic restrictions as well as parsing restrictions because of type deduction. Even if you don't support templates (yet), there's still auto, decltype, __typeof__, and __auto_type (I probably missed some, lol) to worry about.

Also... can HLSL be combined with Objective-C mode (do we have to worry about their pointers as well)?

clang/test/ParserHLSL/parse_pointer.hlsl
10 ↗(On Diff #420670)
12 ↗(On Diff #420670)

I am proud of you for this test case, because I was going to ask for it. :-D

23 ↗(On Diff #420670)

Here are a few more fun test cases (which may require semantic tests instead of parsing tests):

void func();

void devilish_language(auto look_ma_no_pointers); // templates get you the same effects as well

void test() {
  int x;
  devilish_language(&x); // I presume we want this to be diagnosed
  devilish_language(func); // I think you wanted this to not be diagnosed?
  devilish_language("oh no, not array decay!"); // Same here as function decay?

  auto but_but_but = "please, stop Aaron!" + 0; // Uhhh... is the addition okay but the declaration bad?

  int array[2];
  *(&array[0] + 1) = 12; // Maaaayybbbe this is fine?
}

(Thing for you to double-check: do any targets predefine macros that expand to a pointer? I checked InitPreprocessor.cpp and things looked fine there, but I didn't check targets.)

clang/test/ParserHLSL/parse_reference.hlsl
4 ↗(On Diff #420670)
18 ↗(On Diff #420670)

Similar extra tests involving type deduction as the pointer tests.

beanz added a comment.Apr 6 2022, 1:41 PM

I think you're going to need semantic restrictions as well as parsing restrictions because of type deduction. Even if you don't support templates (yet), there's still auto, decltype, __typeof__, and __auto_type (I probably missed some, lol) to worry about.

Thankfully HLSL is like C++98-based... I don't think we support any of those cases.

Also... can HLSL be combined with Objective-C mode (do we have to worry about their pointers as well)?

Oh god... Now you're giving me nightmares. I feel like implementing objc_msgsend on a GPU might be a bridge (or ten) too far...

clang/test/ParserHLSL/parse_pointer.hlsl
23 ↗(On Diff #420670)

I am in awe of your gift for finding ways to trip things up :)

I think most of these we can't hit because of auto. Eyeballing that lat case we _might_ handle it. I'll double check.

I think you're going to need semantic restrictions as well as parsing restrictions because of type deduction. Even if you don't support templates (yet), there's still auto, decltype, __typeof__, and __auto_type (I probably missed some, lol) to worry about.

Thankfully HLSL is like C++98-based... I don't think we support any of those cases.

Oh, were compilers that easy... https://godbolt.org/z/EMasKrdh3

Also... can HLSL be combined with Objective-C mode (do we have to worry about their pointers as well)?

Oh god... Now you're giving me nightmares. I feel like implementing objc_msgsend on a GPU might be a bridge (or ten) too far...

Heh, good. Can you make sure we reject that situation loudly in the frontend (in another patch, on another day, no rush)?

clang/test/ParserHLSL/parse_pointer.hlsl
23 ↗(On Diff #420670)

I am broken for writing decent code after spending so many years in Core thinking about edge cases. :-D

Btw, here's another one to try:

int i = *(int *)0; // Check that pointer types used in expressions are caught, same for reference

Do you need a SemaHLSL.cpp file similar to SemaSYCL.cpp?

beanz added a comment.Apr 6 2022, 1:59 PM

Do you need a SemaHLSL.cpp file similar to SemaSYCL.cpp?

Yes absolutely. I haven't yet added any Sema-specific validations yet that are complex enough to break out, but we will 100% get there soon.

Do you need a SemaHLSL.cpp file similar to SemaSYCL.cpp?

Yes absolutely. I haven't yet added any Sema-specific validations yet that are complex enough to break out, but we will 100% get there soon.

Is the downstream compiler also relying on the parser?

beanz added a comment.Apr 6 2022, 2:03 PM

Is the downstream compiler also relying on the parser?

Not sure what you mean by downstream compiler. The current official HLSL compiler is a fork of clang-3.7. We don't intend to merge code in either direction. The fork does have modifications throughout clang including the parser, sema, and codegen layers.

Relying on the parser to find semantic misbehavior sounds strange ...

beanz added a comment.Apr 6 2022, 2:07 PM

Relying on the parser to find semantic misbehavior sounds strange ...

I don't disagree. An alternative point: HLSL has no _syntax_ for pointers.

Relying on the parser to find semantic misbehavior sounds strange ...

I don't disagree. An alternative point: HLSL has no _syntax_ for pointers.

I got your point.

Relying on the parser to find semantic misbehavior sounds strange ...

I had a meeting with @beanz today because I was also pretty surprised. Chris, it might make sense to type up a bit of what we talked about as an FAQ and maybe stick it in Clang's docs directory; WDYT?

beanz added a comment.Apr 6 2022, 2:10 PM

I had a meeting with @beanz today because I was also pretty surprised. Chris, it might make sense to type up a bit of what we talked about as an FAQ and maybe stick it in Clang's docs directory; WDYT?

Totally agree. I had actually started working on something to help bring new contributors into the effort. I'll try to get something up this week.

I had a meeting with @beanz today because I was also pretty surprised. Chris, it might make sense to type up a bit of what we talked about as an FAQ and maybe stick it in Clang's docs directory; WDYT?

Totally agree. I had actually started working on something to help bring new contributors into the effort. I'll try to get something up this week.

Huttah! Thank you. :-)

beanz updated this revision to Diff 422738.Apr 13 2022, 9:15 PM

Updating based on PR feedback.

I've moved the error reporting into Sema, and added additional errors for unsupported operators.

I've also expanded the test coverage with some of @aaron.ballman's cursed code, which improves coverage substantailly.

beanz edited the summary of this revision. (Show Details)Apr 13 2022, 9:17 PM
aaron.ballman added inline comments.Apr 14 2022, 6:08 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11579–11584

Should we be adding .* and ->* to the list of unsupported operators? (Hmmm, or can we never get to those operators because we can't form the base expression anyway?)

clang/lib/Sema/SemaExpr.cpp
15252–15257

How should this interplay with overloaded operators on user-defined types? Is that case allowed so long as it doesn't form a pointer or a reference? (We should add test coverage for that.)

clang/lib/Sema/SemaExprMember.cpp
1739–1740

Same question here for overloaded operators.

clang/test/SemaHLSL/prohibit_reference.hlsl
4

void chirp(int &&); // error

beanz added a comment.Apr 14 2022, 7:01 AM

One comment inline. Update coming in a few minutes.

clang/lib/Sema/SemaExpr.cpp
15252–15257

So, the "correct" end goal is that we don't support overloading those operators. We have a few limitations on which operators we allow overloading for, generally any operators that in normal use return references or pointers, we don't allow you to overload.

That said, I wrote some test code to see where this falls over with my current change:

struct Fish {
  struct Fins {
    int Left;
    int Right;
  };
  int X;
  int operator *() {
    return X;
  }

  Fins operator ->() {
    return Fins();
  }
};

int gone_fishing() {
  Fish F;
  int Result = *F;       // works... and is consistent with DXC
  Result += F->Left; // error: member reference type 'Fish::Fins' is not a pointer
  return Result;
}

In the existing compiler we produce an error on definition of operators that aren't supported. I'd like to handle improving the diagnostics for those operators that way in a later patch if that is okay.

The .* and .-> operators I couldn't get errors to actually trigger because I couldn't think of a way to write them that wasn't dependent on getting a member pointer, which errors and causes the remaining expression to not be checked.

beanz updated this revision to Diff 422860.Apr 14 2022, 7:16 AM

Updates to test cases to increase coverage.

aaron.ballman accepted this revision.Apr 14 2022, 10:47 AM

LGTM aside from the formatting nit.

clang/lib/Sema/SemaExpr.cpp
15252–15257

In the existing compiler we produce an error on definition of operators that aren't supported. I'd like to handle improving the diagnostics for those operators that way in a later patch if that is okay.

That's fine by me.

The .* and .-> operators I couldn't get errors to actually trigger because I couldn't think of a way to write them that wasn't dependent on getting a member pointer, which errors and causes the remaining expression to not be checked.

Perfect, I couldn't find a way either, we can punt on that until we find a concrete issue.

This revision is now accepted and ready to land.Apr 14 2022, 10:47 AM
This revision was landed with ongoing or failed builds.Apr 14 2022, 11:33 AM
This revision was automatically updated to reflect the committed changes.