-
Notifications
You must be signed in to change notification settings - Fork 351
Split overloaded uses of RefType in front-end #8427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split overloaded uses of RefType in front-end #8427
Conversation
Overview ======== This change is the start of an attempt to address how the Slang compiler codebase has ended up conflating two similar, but semantically distinct, concepts: * The long-standing notion of `ref` parameters (only allowed for use in the builtin modules), which are encoded using a wrapper `Type` in the AST as part of the representation of the parameters of a `FuncType`. * A recently-introduced notion of explicit reference types that mirror the built-in `Ptr` type, with a relationship comparable to that between pointer and reference types in C++. The change splits the `Ref<T>` type in the core module into two distinct types, with one for each of the two use cases. Similarly, the `RefType` class in the compiler's AST is split into two distinct classes, to represent the two cases. Background ========== The `Ref<T>` type in the core module (hidden and not intended for users to ever see or use) was originally introduced to encode the `ref` parameter-passing mode, comparable to the hidden `Out<T>` and `InOut<T>` types used to encode `out` and `inout` parameter-passing modes. The `Ref<T>` type in the core module was encoded as a instance of the `RefType` class in the Slang AST (similar to how `Out<T>` mapped to an `OutType`). These AST classes were *only* intended to be used by the compiler front-end as part of its encoding of function types. The `FuncType` class needed a way to distinguish an `inout int` parameter from a plain (implicitly `in`) `int` parameter, so these wrapper like `RefType` and `OutType` were introduced to encode both the parameter type (`T`) and the parameter-passing mode in a form that could be passed around as a `Type`. Notably, the `Ref<T>` type (and `Out<T>`, etc.) were *not* intended to be type names that ever get uttered in Slang code (not even in the builtin modules), and the vast majority of the compiler code was not supposed to ever encounter them. They were an implementation detail of `FuncType`, and nothing else. (In hindsight it may have been a mistake to use a nominal type declared in the core module to implement these wrappers; it might have been a good idea to use an entirely separate class of `Type` for this case...) Recent changes to the builtin modules introduced functions that wanted to *return* a reference (so that the parameter-passing-mode modifiers like `ref` could not trivially be used), and as part of those changes the appealingly-named `Ref<T>` type in the core module was re-used for this new case. Builtin operations were declared with an explicit `Ref<T>` return type, and parts of the compiler front-end that had previously been blissfully unaware of the AST's `RefType` (and `InOutType`, etc.) had to start accounting for the possibility that an explicit `Ref<T>` would show up. Related changes also introduced a comparable conflation of the (unfortunately-named) `constref` parameter-passing modifier and builtin operations that wanted to return an explicit reference that is read-only. Both use cases were mapped to the core-module `ConstRef<T>` type, which appeared in the AST as an instance of the `ConstRefType` class. The overlapping use of `ConstRef<T>`` is actually significantly more troublesome than the `Ref<T>` case because, despite what its name implies, `constref` was not really supposed to be the read-only analogue of `ref`, but rather it is closer to the "immutable value borrow" analogue to `inout`'s "mutable value borrow." The semantics of a "value borrow" vs. a "memory reference" in Slang have not been very carefully codified, and the conflation around `ConstRef<T>` has contributed to things becoming increasingly muddy in the compiler back-end. Main Changes ============ Core Module ----------- The `Ref<T>` type has been replaced with two distinct types, with one for each use case: * `RefParam<T>` is intended for use when encoding a `ref` parameter in a function type * `ExplicitRef<T>` is intended for use when an operation in a builtin module wants to return a reference The other types used to represent parameter-passing modes (e.g., `InOut<T>`) were renamed to better indicate that their role in defining parameter types (e.g., `InOutParam<T>`). The `ExplicitRef<T>` type was given additional generic parameters for the allowed access and the address space, akin to what `Ptr<T>` now supports. The pointer dereference operator (prefix `*`) in the core module should now properly propagate the access and address space of the pointer over to the reference that gets returned. The two distinct use cases of `ConstRef<T>` were not split in the way as `Ref<T>`, instead the case for the `constref` parameter-passing mode uses `ConstParamRef<T>`, while cases that previously used `ConstRef<T>` to represent a read-only explicit reference instead now use `ExplicitRef<T, Access.Read>`. Prior to this change there were two subscripts declared on pointers: one in the `Ptr` type itself, and another in an `extension` for pointers with `Access.ReadWrite`. The comments on the code seemed to indicate that the catch-all subscript used to only have a `get` accessor, while the `ref` was only available on read-write pointers, but it seems that subsequent changes converted the default subscript to support `ref`. This change eliminates the subscript added via `extension`, since it is redundant. AST and Front-End ================= Similar to the changes in the core module, the AST `RefType` class was split into: * `RefParamType` for the case of encoding `ref` parameters * `ExplicitRefType` for the case where the user meant an explicit reference type All the other classes that represent wrappers for encoding parameter-passing modes (e.g., `OutType`) were similarly renamed (e.g., `OutParamType`). The `ConstRefType` class was simply renamed to `ConstRefParamType`, because any use cases of `ConstRefType` that intended an explicit reference type will now use `ExplicitRefType` with `Acccess.Read`. For convenience, this change includes type aliases to map the old names for these types over to the new ones (e.g., `using OutType = OutParamType`) so that the change doesn't need to affect quite so many lines of code. The `RefType` and `ConstRefType` names are intentionally left undefined, since it woudl be unsafe to assume that existing use sites should default to either of the two possible interpretations. All use cases of `RefType` and `ConstRefType` (and their former shared base class `RefTypeBase`) were audited and updated to refer to either `RefParamType`/`ConstRefParamType` or `ExplicitRefType`, as appropriate (based on whether the context of the code indicated it was working with parameter-passing mode wrapper types, or explicit reference types). In many (many) cases comments were added to the code that was updated (and some unrelated code that needed to be audited along the way) to note cases where there appears to be something fishy going on in the compiler and/or there are obvious opportunities for next-step improvement. The `QualType` constructor used to infer l-value-ness when passed a `RefType` or `ConstRefType`; that code was introduced to support explicit reference types. The code was updated to consult the access argument of an `ExplicitRefType` to try and determine the right l-value-ness to use. There is some ambiguity about what should be done in the case where the value of the generic argument representing the access cannot be statically determined; a better solution may be needed. Many other cases in the front-end that were working with `RefType` and `ConstRefType` for explicit references also need to figure out l-value-ness, and these were changed to rely on the logic already added to `QualType` so that it wouldn't have to be duplicated. It isn't clear if this structure is the best way to tackle the problem, but it seems to at least be an upgrade over the more strictly ad-hoc logic that was in place before. Future Work =========== IR-Level Work ------------- The most obvious next step to take is that the split that was made in the compiler front-end needs to be properly plumbed through all of the back-end. There appears to be a lot of code in the back end of the compiler that has made the same conflation of `ref` parameters and explicit reference types that the front-end did. In practice, any uses of `ExplicitRef<T>` in the front-end should desugar into plain pointer-based code in the IR. Clean Up Parameter-Passing Modes -------------------------------- The code that handles different parameter-passing modes (`ParameterDirection`s) and their wrapper types is somewhat scattered and messy (as found while auditing use cases of `RefType`). A cleanup pass is warranted to ensure that most code only needs to think about `ParameterDirection`s. There should ideally be only a single operation in the front-end that handles determining the `ParameterDirection` of a parameter based on its modifiers. Similarly, there should be one operation to wrap a value type based on a parameter direction, and one operation to derive a `ParameterDirection` from the wrapper type. Ideally, the accessors for `FuncType` should not provide unrestricted access to the potentially-wrapped parameter types, and should instead return some kind of `ParamInfo` struct that encodes both a `ParameterDirection` and the unwrapped `Type` of the parameter. Clean Up `QualType` ------------------- A significant piece of future work that appears required is to drastically clean up and improve the way that `QualType`s are represente and handled in the front-end. There are currently various distinct `bool` flags in `QualType` (some with very unclear meaning) and differnet parts of the codebase consult/modify only subsets of them; a clear enumeration of the "value categories" (to use the C++ terminology) that Slang supports could be quite helpful. Naively, a `QualType` should at least encode the basic information that a `Ptr` type encodes: * A value type * Allowed access (read-only, read-write, etc.) * Address space The main additional thing that a `QualType` needs is a way to distinguish cases where an expression evaluates to: * A reference to a memory location, where all the information from a `Ptr` is relevant * A simple value, such that the access and address space are irrelevant * A reference to an abstract storage location (a `property`, `subscript`, or an implicit conversion that needs to support being an l-value), in which case address space is irrelevant and the "allowed access" basically amounts to a listing of the accessors the storage location supports Eliminate Explicit Reference Types ---------------------------------- Finally, twe should eventually eliminate the `ExplicitRef<T>` type from the core module (and all of the supporting code from the front-end), since the feature is not a good fit for the Slang language. We should find some other way to decorate operations in the builtin module that need to returns a reference rather than a value (note how `ref` accessors already avoided exposing explicit reference types, by design).
/format |
🌈 Formatted, please merge the changes from this PR |
…type-usage Format code for PR shader-slang#8427
source/slang/core.meta.slang
Outdated
} | ||
}; | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this extension just feels so wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll delete it once I can confirm that everything is passing CI.
The central problem is that the basic Ptr
type wanted to provide a read-only subscript by default (leaving aside the question of whether it should support subscripting in the first place...), so the ref
accessor was factored out into an extnesion
, but then we (seemingly) realized that even in the read-only case we wanted subscripting to return a reference, and not a value, and we don't have any kind of __constref
accessor right now, so the only option was to switch the basic case to a ref
.
Hacking on this PR has definitely left me questioning whether it works best to have the access (read-only, write-only, or read-write) of a pointer be encoded as a generic argument, or whether that should be modeled with distinct pointer types (Ptr
vs. MutablePtr
or whatever).
source/slang/core.meta.slang
Outdated
|
||
// By default, getter is not an L value | ||
// | ||
// TODO(tfoley): Yes... yes it *is* an l-value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah let's delete that silly comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. I left a lot of comments in this draft that probably need a cleanup pass before I commit, because I wanted to spark the necessary conversations.
source/slang/hlsl.meta.slang
Outdated
|
||
__intrinsic_op($(kIROp_ResolveVaryingInputRef)) | ||
Ref<T> __ResolveVaryingInputRef<T>(__constref T attribute); | ||
ExplicitRef<T, Access.Read> __ResolveVaryingInputRef<T>(__constref T attribute); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can provide an explicit address space that correspond to varying input here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that might be good, or we could at least pass in the Generic
address space instead of letting it using the default (Device
).
This function also is one of the examples where we are using __constref
to mean a read-only __ref
parameter, while we have plenty of other cases where it is being used to mean the read-only analogue of inout
. So even if this PR cleaves off the overloading of Ref<T>
, we still have to deal with the overloaded meaning of __constref
soon.
Related: I'm not convinced that the overall approach of handling various features related to varying inputs using explicit references in the core module is the right way. At the very least, going down this road pretty much dictates that we add the ability to attach an explicit address space to a ref
parameter, which starts to move us in a messy direction, syntactically.
return true; | ||
} | ||
|
||
// TODO(tfoley): I was told that explicit `Ref` types should not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what will happen if we remove the coercion logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm interested in many of these questions too, but I've intentionally tried to force myself to stay on task with this PR and not engage in unrelated experiments or clean-ups (because those have a tendency to cause my PRs to balloon out of control).
// If the base isn't a pointer, then we are trying to form | ||
// a const ref to a temporary value. | ||
// To do so we must copy it into a variable. | ||
// If the base expression is not one that (trivially) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could be lacking necessary front end check when passing an abstract storage decl to a __ref
parameter. If a user attempts that we may be triggering the assert here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is specifically related to __constref
parameters, in some way. I may still be missing something, though, because I'm seeing code in addArg()
in slang-lower-to-ir.cpp
that is checking for a parameter with a type that is an IRConstRefType
and that code is triggering, even though I wouldn't expect to see that appear as the type of a parameter at that point.
// TODO(tfoley): This code implements the `(T,bool)` ordering, | ||
// which provides more easy opportunities to generate compact | ||
// layouts by using "tail padding" than the `(bool, T)` ordering. | ||
// However the "natural layout" implementation does not match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which implementation are you refering to? The IR pass the lowers optional to struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In slang-ast-natural-layout.cpp
:
NaturalSize size = NaturalSize::makeEmpty();
size.append(calcSize(m_astBuilder->getBoolType()));
size.append(calcSize(optionalType->getValueType()));
So, in debugging the test failures that were arising with this change I think I ran into (and fixed) a major bug in how AST-to-IR lowering was being performed for function calls to anything that wasn't a The issue has clearly been around for quite a while (perhaps since the earliest versions of the AST-to-IR pass), and I have seen various local band-aid fixes in IR passes indicating that developers were running into the relevant issues (things like the IR trying to use a value of type |
/format |
🌈 Formatted, please merge the changes from this PR |
/format |
🌈 Formatted, please merge the changes from this PR |
…type-usage Format code for PR shader-slang#8427
…internal-use-only type as if it is part of the public API of the core module
The failure of the
As far as I can tell, that is some kind of random/intermittent failure that has nothing to do with my changes, since I didn't touch that file and certainly didn't do anything that would cause |
Overview
This change is the start of an attempt to address how the Slang compiler codebase has ended up conflating two similar, but semantically distinct, concepts:
The long-standing notion of
ref
parameters (only allowed for use in the builtin modules), which are encoded using a wrapperType
in the AST as part of the representation of the parameters of aFuncType
.A recently-introduced notion of explicit reference types that mirror the built-in
Ptr
type, with a relationship comparable to that between pointer and reference types in C++.The change splits the
Ref<T>
type in the core module into two distinct types, with one for each of the two use cases. Similarly, theRefType
class in the compiler's AST is split into two distinct classes, to represent the two cases.Background
The
Ref<T>
type in the core module (hidden and not intended for users to ever see or use) was originally introduced to encode theref
parameter-passing mode, comparable to the hiddenOut<T>
andInOut<T>
types used to encodeout
andinout
parameter-passing modes. TheRef<T>
type in the core module was encoded as a instance of theRefType
class in the Slang AST (similar to howOut<T>
mapped to anOutType
). These AST classes were only intended to be used by the compiler front-end as part of its encoding of function types. TheFuncType
class needed a way to distinguish aninout int
parameter from a plain (implicitlyin
)int
parameter, so these wrapper likeRefType
andOutType
were introduced to encode both the parameter type (T
) and the parameter-passing mode in a form that could be passed around as aType
.Notably, the
Ref<T>
type (andOut<T>
, etc.) were not intended to be type names that ever get uttered in Slang code (not even in the builtin modules), and the vast majority of the compiler code was not supposed to ever encounter them. They were an implementation detail ofFuncType
, and nothing else.(In hindsight it may have been a mistake to use a nominal type declared in the core module to implement these wrappers; it might have been a good idea to use an entirely separate class of
Type
for this case...)Recent changes to the builtin modules introduced functions that wanted to return a reference (so that the parameter-passing-mode modifiers like
ref
could not trivially be used), and as part of those changes the appealingly-namedRef<T>
type in the core module was re-used for this new case. Builtin operations were declared with an explicitRef<T>
return type, and parts of the compiler front-end that had previously been blissfully unaware of the AST'sRefType
(andInOutType
, etc.) had to start accounting for the possibility that an explicitRef<T>
would show up.Related changes also introduced a comparable conflation of the (unfortunately-named)
constref
parameter-passing modifier and builtin operations that wanted to return an explicit reference that is read-only. Both use cases were mapped to the core-moduleConstRef<T>
type, which appeared in the AST as an instance of theConstRefType
class.The overlapping use of
ConstRef<T>`` is actually significantly more troublesome than the
Refcase because, despite what its name implies,
constrefwas not really supposed to be the read-only analogue of
ref, but rather it is closer to the "immutable value borrow" analogue to
inout's "mutable value borrow." The semantics of a "value borrow" vs. a "memory reference" in Slang have not been very carefully codified, and the conflation around
ConstRef` has contributed to things becoming increasingly muddy in the compiler back-end.Main Changes
Core Module
The
Ref<T>
type has been replaced with two distinct types, with one for each use case:RefParam<T>
is intended for use when encoding aref
parameter in a function typeExplicitRef<T>
is intended for use when an operation in a builtin module wants to return a referenceThe other types used to represent parameter-passing modes (e.g.,
InOut<T>
) were renamed to better indicate that their role in defining parameter types (e.g.,InOutParam<T>
).The
ExplicitRef<T>
type was given additional generic parameters for the allowed access and the address space, akin to whatPtr<T>
now supports. The pointer dereference operator (prefix*
) in the core module should now properly propagate the access and address space of the pointer over to the reference that gets returned.The two distinct use cases of
ConstRef<T>
were not split in the way asRef<T>
, instead the case for theconstref
parameter-passing mode usesConstParamRef<T>
, while cases that previously usedConstRef<T>
to represent a read-only explicit reference instead now useExplicitRef<T, Access.Read>
.Prior to this change there were two subscripts declared on pointers: one in the
Ptr
type itself, and another in anextension
for pointers withAccess.ReadWrite
. The comments on the code seemed to indicate that the catch-all subscript used to only have aget
accessor, while theref
was only available on read-write pointers, but it seems that subsequent changes converted the default subscript to supportref
. This change eliminates the subscript added viaextension
, since it is redundant.AST and Front-End
Similar to the changes in the core module, the AST
RefType
class was split into:RefParamType
for the case of encodingref
parametersExplicitRefType
for the case where the user meant an explicit reference typeAll the other classes that represent wrappers for encoding parameter-passing modes (e.g.,
OutType
) were similarly renamed (e.g.,OutParamType
).The
ConstRefType
class was simply renamed toConstRefParamType
, because any use cases ofConstRefType
that intended an explicit reference type will now useExplicitRefType
withAcccess.Read
.For convenience, this change includes type aliases to map the old names for these types over to the new ones (e.g.,
using OutType = OutParamType
) so that the change doesn't need to affect quite so many lines of code. TheRefType
andConstRefType
names are intentionally left undefined, since it woudl be unsafe to assume that existing use sites should default to either of the two possible interpretations.All use cases of
RefType
andConstRefType
(and their former shared base classRefTypeBase
) were audited and updated to refer to eitherRefParamType
/ConstRefParamType
orExplicitRefType
, as appropriate (based on whether the context of the code indicated it was working with parameter-passing mode wrapper types, or explicit reference types).In many (many) cases comments were added to the code that was updated (and some unrelated code that needed to be audited along the way) to note cases where there appears to be something fishy going on in the compiler and/or there are obvious opportunities for next-step improvement.
The
QualType
constructor used to infer l-value-ness when passed aRefType
orConstRefType
; that code was introduced to support explicit reference types. The code was updated to consult the access argument of anExplicitRefType
to try and determine the right l-value-ness to use. There is some ambiguity about what should be done in the case where the value of the generic argument representing the access cannot be statically determined; a better solution may be needed.Many other cases in the front-end that were working with
RefType
andConstRefType
for explicit references also need to figure out l-value-ness, and these were changed to rely on the logic already added toQualType
so that it wouldn't have to be duplicated. It isn't clear if this structure is the best way to tackle the problem, but it seems to at least be an upgrade over the more strictly ad-hoc logic that was in place before.Future Work
IR-Level Work
The most obvious next step to take is that the split that was made in the compiler front-end needs to be properly plumbed through all of the back-end. There appears to be a lot of code in the back end of the compiler that has made the same conflation of
ref
parameters and explicit reference types that the front-end did. In practice, any uses ofExplicitRef<T>
in the front-end should desugar into plain pointer-based code in the IR.Clean Up Parameter-Passing Modes
The code that handles different parameter-passing modes (
ParameterDirection
s) and their wrapper types is somewhat scattered and messy (as found while auditing use cases ofRefType
). A cleanup pass is warranted to ensure that most code only needs to think aboutParameterDirection
s. There should ideally be only a single operation in the front-end that handles determining theParameterDirection
of a parameter based on its modifiers. Similarly, there should be one operation to wrap a value type based on a parameter direction, and one operation to derive aParameterDirection
from the wrapper type. Ideally, the accessors forFuncType
should not provide unrestricted access to the potentially-wrapped parameter types, and should instead return some kind ofParamInfo
struct that encodes both aParameterDirection
and the unwrappedType
of the parameter.Clean Up
QualType
A significant piece of future work that appears required is to drastically clean up and improve the way that
QualType
s are represente and handled in the front-end. There are currently various distinctbool
flags inQualType
(some with very unclear meaning) and differnet parts of the codebase consult/modify only subsets of them; a clear enumeration of the "value categories" (to use the C++ terminology) that Slang supports could be quite helpful. Naively, aQualType
should at least encode the basic information that aPtr
type encodes:The main additional thing that a
QualType
needs is a way to distinguish cases where an expression evaluates to:Ptr
is relevantproperty
,subscript
, or an implicit conversion that needs to support being an l-value), in which case address space is irrelevant and the "allowed access" basically amounts to a listing of the accessors the storage location supportsEliminate Explicit Reference Types
Finally, twe should eventually eliminate the
ExplicitRef<T>
type from the core module (and all of the supporting code from the front-end), since the feature is not a good fit for the Slang language. We should find some other way to decorate operations in the builtin module that need to returns a reference rather than a value (note howref
accessors already avoided exposing explicit reference types, by design).