[PATCH] D123278: [Clang] [Docs] Add HLSLSupport page

Fri Apr 8 08:11:02 PDT 2022

beanz added inline comments.

================
Comment at: clang/docs/HLSLSupport.rst:150-152
+HLSL has a ``precise`` qualifier that behaves unlike anything else in the C
+language. The support for this qualifier in DXC is buggy, so our bar for
+compatibility is low.
----------------
aaron.ballman wrote:
> beanz wrote:
> > aaron.ballman wrote:
> > > beanz wrote:
> > > > aaron.ballman wrote:
> > > > > Is it worth supporting at all (I know you want source compatibility...)? Type system changes are generally expensive and invasive, largely because changing the type system in C++ is complicated. For example, does this qualifier impact overload sets or template specializations? Does it get name mangled? That sort of thing.
> > > > Unfortunately we do need to implement it to some degree. In DXC it is implemented as an attribute rather than a qualifier, so it shouldn't impact overloads, template specialization or mangling.
> > > > 
> > > > I call it a qualifier here because it is syntactically more like a qualifier, but I'm honestly not sure how we should implement it. In DXC, we emit IR metadata denoting the underlying value as "precise", then in an IR pass propagate disabling fast math.
> > > > Unfortunately we do need to implement it to some degree. In DXC it is implemented as an attribute rather than a qualifier, so it shouldn't impact overloads, template specialization or mangling.
> > > 
> > > Okay, I kind of figured you needed it.
> > > 
> > > > I call it a qualifier here because it is syntactically more like a qualifier, but I'm honestly not sure how we should implement it. In DXC, we emit IR metadata denoting the underlying value as "precise", then in an IR pass propagate disabling fast math.
> > > 
> > > This smells more like an attribute than a qualifier to me, but I'd like to see if I understand first (please ignore if I get syntax wrong):
> > > ```
> > > // Initialization
> > > precise float f = 1.0 / 3.0 * sinf(3.14f); // value computation is precise
> > > // Assignment
> > > f = 1.0 / 3.0 * sinf(3.14f); // value computation is NOT precise (not a declaration)?
> > > // RHS of expression
> > > float f2 = f * 2.0; // value computation is NOT precise (f2 is not marked precise)?
> > > ```
> > > 
> > Yea, your syntax and behavior is correct with the added complication that precise upward propagates. The upward propagation is the part that is wonky and problematic.
> > 
> > ```
> > float f = 1.0 * sinf(3.14f); // precise
> > precise float f2 = f / 2.0f; // precise
> > float f3 = f * f2; // not precise
> > ```
> > 
> Sorry if I'm being dense, but I'm still lost. Why is `f` precise in your example?
> 
> But more interestingly, is this a property of the *initialization* of the variable of that type only, or is it a property of the type itself where arbitrary expressions involving that type need to care whether it's precise or not?
> 
> If it's just about the initialization, then I think a declaration attribute is the clear winner for the semantic support. If it needs to be tracked through arbitrary expressions involving the type, it's probably better as a qualifier sort of like the nullability ones. e.g.,
> ```
> template <typename Ty>
> void func(Ty Val) {
>   Val += 1.0f // Does this get precise semantics?
> }
> 
> int main() {
>   precise float f = 1.0f;
>   func(f);
> }
> ```
> 
> 
`precise` behaves unlike anything I've ever encountered in a programming language before. It neither follows the type nor the declared value.

`precise` never propagates down (so a use of a `precise` value does not make subsequent things `precise`. It does propagate up to values that contribute to the `precise`, which is just... weird.

I don't know whether HLSL or GLSL (or some other SL) did it first, but GLSL has a spec which states:

> The **precise** qualifier ensures that operations contributing to a variable's value are done  in their stated order and with operator consistency.

The GLSL spec expands into a whole lot more details and examples:
https://www.khronos.org/registry/OpenGL/specs/gl/GLSLangSpec.4.60.pdf

HLSL's behavior //should// match GLSL (and some other vendor-specific languages), but frequently doesn't.

================
Comment at: clang/docs/HLSLSupport.rst:197
+* Any use of the ``virtual`` keyword
+* Most features C++11 and later
----------------
aaron.ballman wrote:
> beanz wrote:
> > aaron.ballman wrote:
> > > beanz wrote:
> > > > aaron.ballman wrote:
> > > > > Same question here as above with C on whether we expect to support those as extensions or diagnose them as invalid.
> > > > I didn't really answer that above. I am inclined to do a mix of both. Anything that we can support and lower I'd like to treat as extensions. Some things we just can't.
> > > > 
> > > > We don't currently have any way to allocate memory dynamically, so there's really no way to support new/delete. RTTI and exceptions are another case where we just don't have the support we would need in the driver specifications.
> > > Okay, that seems reasonable enough. Should we issue "this is a Clang extension" diagnostics in the places where you're supporting something as an extension? Also, I presume you intend to add error diagnostics for all the cases that are not supported?
> > I think issuing clang extension diagnostics will be super valuable to our users. My expectation is that it will take a few years to transition all of our users from DXC to Clang, and in the meantime if they are supporting both compilers in their codebases diagnostics for extension use will be immensely helpful.
> > 
> > For unsupported features, I think the immovable priority is that we can't succeed compiling and generate invalid code (in our primary case that would be code that can't be run under one of our driver specifications).
> > 
> > Having nice, targeted diagnostics for each unsupported case is the ideal, but there are cases (like with Microsoft attribute parsing on templates), where we fail the compile with diagnostics that are "good enough" or even just in parity to DXC which we may take as an opportunity for future improvement and not a requirement.
> > 
> > Does that make sense? I'm trying not to back myself into a corner of having perfect be the enemy of "better than what we have today".
> > I think issuing clang extension diagnostics will be super valuable to our users.
> 
> +1, I think it'd be useful as well, for exactly that situation.
> 
> > For unsupported features, I think the immovable priority is that we can't succeed compiling and generate invalid code (in our primary case that would be code that can't be run under one of our driver specifications).
> 
> +1, definitely agreed.
> 
> > Having nice, targeted diagnostics for each unsupported case is the ideal, but there are cases (like with Microsoft attribute parsing on templates), where we fail the compile with diagnostics that are "good enough" or even just in parity to DXC which we may take as an opportunity for future improvement and not a requirement.
> >
> > Does that make sense? I'm trying not to back myself into a corner of having perfect be the enemy of "better than what we have today".
> 
> Yes, this makes sense. Mostly, I want to ensure that code which is invalid causes errors in the frontend rather than errors in the backend or at runtime when it comes to unsupported features. So it's not so much about what diagnostics are generated as it is about ensuring diagnostics are generated where necessary.
I think You and I are 100% on the same page here. Fail early with actionable errors, rather than asserting in the backend ;P

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123278/new/

https://reviews.llvm.org/D123278