Big update to flesh out const instance methods

[llvm-beanz]

I started digging a bit into this in DXC and identified a lot of cross-cutting issues that we need to consider in this.

This change updates the proposal to flesh out the behavior and related issues around HLSL const-correctness.

This does identify some source-breaking changes that this would introduce as well as some behavior changes that I think we should introduce in the HLSL built-in types.

[ChristianReinbold]

After this PR has been inactive for almost a year now: Are the plans to bring const qualifiers to instance methods still valid? At the moment, I have to compile a lot of shared code (C++ & HLSL) by removing the const keyword with the preprocessor (#define const) whenever dxc is used as compiler. This comes with some obvious drawback, the biggest afaic being that static const variables do not work anymore. (const-correctness is still guaranteed in the C++-pass)

For the time being, I consider hacking in the tok::kw_const case in Parser::ParseTypeQualifierListOpt such that dxc accepts const qualifiers without ending up with a diagnostic. This approach seems to work for me. This PR however suggests that there is more work to do for proper const-qualifier support.

If I know that at some point in the future const-methods will be supported, I would be more inclined to accept the hack mentioned above as a temporary measure.

[llvm-beanz]

@ChristianReinbold, thank you for your interest in the HLSL language.

After this PR has been inactive for almost a year now: Are the plans to bring const qualifiers to instance methods still valid?

Our team is very small and our focus has been on Shader Model 6.8 rather than HLSL 202x over the last year. Which may seem like there is no progress here, but we are still planning this as a likely feature for the next HLSL language version.

At the moment, I have to compile a lot of shared code (C++ & HLSL)...

This is going to be a really difficult thing to do with HLSL as the language exists today. There's a lot of differences between HLSL and C++, so this use case isn't something we actively support with HLSL today (although we are working to support it in the future).

(const-correctness is still guaranteed in the C++-pass)

I assume this is your justification for why the hack you describe below is "ok", but your specific use case isn't the same as others so it isn't something we can bank on people doing. Most HLSL code that exists today isn't shared source with C++.

For the time being, I consider hacking in the tok::kw_const case in Parser::ParseTypeQualifierListOpt such that dxc accepts const qualifiers without ending up with a diagnostic. This approach seems to work for me.

It works by parsing the const qualifier, but it doesn't fix the underlying issues with overload resolution. Today our overload resolution ignores const-qualification, and we actually allow calling non-const methods on constant objects in some contexts because of hacks on top of hacks...

This PR however suggests that there is more work to do for proper const-qualifier support.

Fundamentally it is a change to the language grammar and syntax. All changes to language grammar should be gated on a language version and/or feature check.

If I know that at some point in the future const-methods will be supported, I would be more inclined to accept the hack mentioned above as a temporary measure.

Two points here:

(1) This isn't the place to talk about an implementation detail. This PR and the underlying proposal are about changing the language, not the compiler implementation.

(2) This hack would basically make it so that people writing code would write it in a way that gave them a reasonable expectation that it worked in one way, but it would actually work differently from expectations. That's the kind of behavior we're trying to remove from HLSL because we have too many instances of it.

I know it isn't satisfying, but the only answer today for people trying to share code between HLSL and C++ is to keep using preprocessor macros to morph the code between the languages. For this specific feature you'll need to wait until HLSL 202x begins development.

[ChristianReinbold]

@llvm-beanz

I know it isn't satisfying, but the only answer today...

Thanks for the detailed answer, which - for me - is perfectly satisfying. My intention when writing this comment was about managing expectations, and in this regard it helped a lot. I now have a better idea which rabbit hole we may continue to go down with the current code-base.

Our team is very small and our focus has been on Shader Model 6.8 rather than HLSL 202x over the last year.

I highly appreciate the work you are putting into HLSL, and I am genuinely impressed how well the feature set of HLSL 2021 worked out in practice so far, for a multitude of reasons. I guess we are just testing the limits with our use case. I hope you are fine with me inquiring here and there about features I would be interested in. The current approach of peeking into HLSL specs and dxc sources whenever struggling with unexpected behavior or compiler bugs / limitations, "fixing" them and upstreaming them whenever possible seems to work quite well. As long as you are happy to spend some time with irregular PRs that have the risk of not accounting for the whole picture and thus heading the wrong way sometimes (in the end my compiler background is limited to non-existant), I probably will continue with this approach.

(1) This isn't the place to talk about an implementation detail.

I am aware. I intended to give you a more complete picture about the implications this language feature has for us. I am sorry for having side-tracked the conversation with that.

(2) This hack would basically make it so that people writing code would write it in a way that gave them a reasonable expectation that it worked in one way, but it would actually work differently from expectations.

I agree. I do not consider bringing in this hack as a PR to the dxc repo.

[tex3d]

I think this is good, but I have thoughts on the interpretation of const for resources.

I think it's more consistent, and it unlocks useful scenarios, if we change the interpretation to apply const only to the resource variable (handle) itself, and not to the UAV resource contents pointed to by the variable.

[tex3d]

It would be possible to consider all access operations on resource objects const, since these are not modifying the resource handle itself, which is what the variable contains.

I understand the precedent for replicating const-ness to accesses of the data pointed to by the handle, and even FXC has this same interpretation in this very case.

However, there is a potential advantage to an approach where const on a resource means that the resource handle cannot be modified, while still allowing modification of the contents of the resource it points to (based on resource type being UAV). Consider the following use cases:

// CBufferValue is implicitly const, since it maps to a cbuffer value which cannot be modified.
uint CBufferValue;

// RWBuf may not be reassigned, though there is no obvious language-level
// restriction that indicates this.
// if this could be 'const', while still allowing write operations to the resource,
// it would prevent reassignment naturally at the language-level.
RWBuffer<int> RWBuf;

void fn(uint idx) {
  RWBuffer<int> buf = ResourceDescriptorHeap[5];
  ...
  // Uses of 'buf' follow, but you want to prevent reassigning of the 'buf' resource variable.
  // If the declaration could be const while allowing writes to the resource,
  // this could be achieved easily.
  // This could matter even more as we introduce references to HLSL
}

Note that the following is still perfectly legal with this proposal and FXC:

void setValue(RWBuffer<int> R, int Val, int Index) {
  R[Index] = Val;
}
void setValueConst(const RWBuffer<int> R, int Val, int Index) {
  setValue(R, Val, Index);
}

If it's not legal to write to the resource pointed to by const RWBuffer<int> R, why would it be legal to simply reassign or pass the variable to one without const, then write to the resource that way? This must work because the variable does not hold the resource contents, it's just like a pointer. So why artificially restrict accesses to the resource pointed to, which prevents const usages that could actually be useful to the language from being used (such as ones proposed above)? If we did this, using const on resource variables could become common/recommended practice, to help catch instances of reassignment that could complicate or even prevent compilation.