-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[HLSL] Implement array temporary support #79382
Conversation
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-clang-modules Author: Chris B (llvm-beanz) ChangesIn HLSL function parameters are passed by value, including array parameters. This change introduces a new AST node to represent array temporary expressions. They behave as lvalues to temporary arrays and decay to pointers for overload resolution and code generation. The behavior of HLSL function calls is documented in the draft language specification under the Expr.Post.Call heading. Additionally the design of this implementation approach is documented in Clang's documentation Full diff: https://github.com/llvm/llvm-project/pull/79382.diff 20 Files Affected:
|
@llvm/pr-subscribers-hlsl Author: Chris B (llvm-beanz) ChangesIn HLSL function parameters are passed by value, including array parameters. This change introduces a new AST node to represent array temporary expressions. They behave as lvalues to temporary arrays and decay to pointers for overload resolution and code generation. The behavior of HLSL function calls is documented in the draft language specification under the Expr.Post.Call heading. Additionally the design of this implementation approach is documented in Clang's documentation Full diff: https://github.com/llvm/llvm-project/pull/79382.diff 20 Files Affected:
|
@llvm/pr-subscribers-clang-codegen Author: Chris B (llvm-beanz) ChangesIn HLSL function parameters are passed by value, including array parameters. This change introduces a new AST node to represent array temporary expressions. They behave as lvalues to temporary arrays and decay to pointers for overload resolution and code generation. The behavior of HLSL function calls is documented in the draft language specification under the Expr.Post.Call heading. Additionally the design of this implementation approach is documented in Clang's documentation Full diff: https://github.com/llvm/llvm-project/pull/79382.diff 20 Files Affected:
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang/lib/AST/Expr.cpp
Outdated
@@ -3569,6 +3569,7 @@ bool Expr::HasSideEffects(const ASTContext &Ctx, | |||
case ConceptSpecializationExprClass: | |||
case RequiresExprClass: | |||
case SYCLUniqueStableNameExprClass: | |||
case HLSLArrayTemporaryExprClass: |
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 looks wrong; I think you need the recursive visit here?
clang/lib/Sema/SemaInit.cpp
Outdated
if (LangOpts.HLSL) | ||
if (auto AdjTy = dyn_cast<DecayedType>(Entity.getType())) | ||
if (AdjTy->getOriginalType()->isConstantArrayType()) | ||
InitE = HLSLArrayTemporaryExpr::Create(getASTContext(), InitE); |
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 implementation is surprising. If arrays are supposed to be passed by value, decay shouldn't be happening in the first place.
Allowing decay to happen, then trying to fix it up later, has some weird implications: for example, void f(int[2])
and void f(int[4])
have the same type. You introduce pointer types in places where they shouldn't really exist, and casts which aren't really supposed to happen. And it's not clear to me the non-canonical type wrapper will be preserved in all the cases you need (preserving non-canonical types is best-effort; sometimes we're forced to canonicalize).
As an alternative approach, maybe we can introduce a new type to represent a "function argument of array type". When we construct the function type, instead of decaying to a pointer, it would "decay" to "function argument of array type". The result is sort of similar, but instead of a non-canonical DecayedType, you have a canonical type which would be reliably preserved across the compiler. For argument passing, initialization would work similarly to what you're doing now.
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.
Ooo. I like this idea a lot. Let me go give that a try. I was trying to avoid completely breaking array decay completely because at some point we’re going to align the language more with C/C++.
d01d3dd
to
762646e
Compare
I force pushed an update here because the new implementation is basically a complete rewrite based on @efriedma-quic's suggestion. I think this is a significantly cleaner approach that more accurately models the behavior of HLSL. |
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 seems like the right direction.
clang/lib/AST/ASTContext.cpp
Outdated
@@ -10900,6 +10936,10 @@ QualType ASTContext::mergeTypes(QualType LHS, QualType RHS, bool OfBlockPointer, | |||
assert(LHS != RHS && | |||
"Equivalent pipe types should have already been handled!"); | |||
return {}; | |||
case Type::ArrayParameter: | |||
if (RHS != LHS) |
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.
It's impossible for RHS and LHS to be equal here.
clang/lib/CodeGen/CGCall.cpp
Outdated
// If an argument is an array paramter expression being passed through. Emit | ||
// the argument to a temporary and pass the temporary as the call arg. | ||
if (auto AT = dyn_cast<ArrayParameterType>(type)) { | ||
args.add(EmitAnyExprToTemp(E), type); |
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 don't think this code should be necessary? E shouldn't be an lvalue or a record type, so we should just fall through to the same code at the end of the function, I think?
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.
Ah, yea, I think I need to fix this a different way.
The case where this happens is:
void fn(float x[2]);
void call(float Arr[2]) {
fn(Arr);
}
In the CallExpr to fn, the argument is an lvalue DeclRefExpr to the ParmVar.
I think the right way to do this is to insert an ImplicitCastExpr for CK_HLSLArrayRValue or CK_LValueToRValue.
clang/lib/CodeGen/CGExprConstant.cpp
Outdated
@@ -1126,6 +1126,7 @@ class ConstExprEmitter : | |||
case CK_NonAtomicToAtomic: | |||
case CK_NoOp: | |||
case CK_ConstructorConversion: | |||
case CK_HLSLArrayRValue: |
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.
We shouldn't accept CK_HLSLArrayRValue casts here (or if we do, we need to do something different with them).
clang/lib/CodeGen/CGExprScalar.cpp
Outdated
@@ -2231,7 +2231,8 @@ Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) { | |||
case CK_UserDefinedConversion: | |||
return Visit(const_cast<Expr*>(E)); | |||
|
|||
case CK_NoOp: { | |||
case CK_NoOp: | |||
case CK_HLSLArrayRValue: { |
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.
It shouldn't be possible to hit this case; a value of ArrayParameterType isn't a scalar.
clang/lib/CodeGen/CGExprComplex.cpp
Outdated
@@ -500,6 +500,7 @@ ComplexPairTy ComplexExprEmitter::EmitCast(CastKind CK, Expr *Op, | |||
case CK_NoOp: | |||
case CK_LValueToRValue: | |||
case CK_UserDefinedConversion: | |||
case CK_HLSLArrayRValue: |
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.
The result of an CK_HLSLArrayRValue can't be a complex type.
clang/lib/CodeGen/CGExprAgg.cpp
Outdated
@@ -1442,6 +1445,7 @@ static bool castPreservesZero(const CastExpr *CE) { | |||
case CK_BitCast: | |||
case CK_ToUnion: | |||
case CK_ToVoid: | |||
case CK_HLSLArrayRValue: |
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.
Probably should be false, like other lvalue-to-rvalue casts?
clang/include/clang/AST/Type.h
Outdated
} | ||
}; | ||
|
||
/// Represents the canonical version of C arrays with a specified constant size. | ||
/// For example, the canonical type for 'int A[4 + 4*100]' is a | ||
/// ConstantArrayType where the element type is 'int' and the size is 404. | ||
class ConstantArrayType final | ||
: public ArrayType, | ||
private llvm::TrailingObjects<ConstantArrayType, const Expr *> { |
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.
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.
The size expression is usually not part of a constant array type. Normally, all we want as part of the type node is the bound, and the size expression if needed can be found on the ArrayTypeLoc
. The rare special case being handled here is that if the array bound is instantiation-dependent but not dependent, then we need to preserve the expression in the type (not just in the TypeLoc
) because it affects (for example) whether two template declarations involving such a type are considered to declare the same template. Because this is a very rare case, and ConstantArrayType
s can be relatively common (eg, as the types of string literals), it seemed preferable to not store the Expr*
if it's not needed, but in absolute terms it's probably a minor difference in memory usage, so it's probably fine to remove this optimization.
If we want to keep the optimization, perhaps we could replace the Size
and SizeExpr
fields with a pointer union that can hold either a <= 63-bit array bound (the common case) or a pointer to a slab-allocated pair of APInt
and const Expr*
(in the case where the bound doesn't fit in 63 bits or the size expression is instantiation-dependent).
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.
@zygoloid do you think the optimization is important to preserve? I guessed it might be safe to eliminate because other array types store the SizeExpr
pointer, but if it's important I can make a separate PR to change the implementation as you suggested and we can merge that change first.
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.
As a general principle we try to keep our AST nodes small. This change isn't going to make a big difference by itself, but abandoning that principle and making a bunch of changes like this would. I think it's worth putting in a small amount of effort here, but if it doesn't work out nicely then it's fine to remove the optimization. We can find other places to save space.
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 PR llvm#79382, I need to add a new type that derives from ConstantArrayType. This means that ConstantArrayType can no longer use `llvm::TrailingObjects` to store the trailing optional Expr*. This change refactors ConstantArrayType to store a 60-bit integer and 4-bits for the integer size in bytes. This replaces the APInt field previously in the type but preserves enough information to recreate it where needed. To reduce the number of places where the APInt is re-constructed I've also added some helper methods to the ConstantArrayType to allow some common use cases that operate on either the stored small integer or the APInt as appropriate. Resolves llvm#85124.
In PR #79382, I need to add a new type that derives from ConstantArrayType. This means that ConstantArrayType can no longer use `llvm::TrailingObjects` to store the trailing optional Expr*. This change refactors ConstantArrayType to store a 60-bit integer and 4-bits for the integer size in bytes. This replaces the APInt field previously in the type but preserves enough information to recreate it where needed. To reduce the number of places where the APInt is re-constructed I've also added some helper methods to the ConstantArrayType to allow some common use cases that operate on either the stored small integer or the APInt as appropriate. Resolves #85124.
HLSL constant sized array function parameters do not decay to pointers. Instead constant sized array types are preserved as unique types for overload resolution, template instantiation and name mangling. This implements the change by adding a new `ArrayParameterType` which represents a non-decaying `ConstantArrayType`. The new type behaves the same as `ConstantArrayType` except that it does not decay to a pointer. Values of `ConstantArrayType` in HLSL decay during overload resolution via a new `HLSLArrayRValue` cast to `ArrayParameterType`. `ArrayParamterType` values are passed indirectly by-value to functions in IR generation resulting in callee generated memcpy instructions.
earlier iteration.
I beleive this captures all of the feedback from @efriedma-quic. Thank you for your patience and great feedback!
7a3130c
to
41ea1e8
Compare
clang/lib/AST/ASTContext.cpp
Outdated
case Type::ArrayParameter: | ||
assert(LHS != RHS && | ||
"Equivalent pipe types should have already been handled!"); | ||
return LHS; |
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.
return {};
? Also, assertion text is wrong.
You could try to merge like we do for ConstantArray... but it's unlikely to matter, since HLSL is a C++ variant (and type merging is basically a C-only thing).
@@ -2331,6 +2332,11 @@ void CodeGenFunction::EmitVariablyModifiedType(QualType type) { | |||
type = cast<DecayedType>(ty)->getPointeeType(); | |||
break; | |||
|
|||
case Type::ArrayParameter: |
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.
Can we just use the existing "case" for ConstantArray?
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.
LGTM
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.
WOW.. This was pretty big. I went through it twice and didn't see anything that jumped out. Looks good to me.
@@ -0,0 +1,76 @@ | |||
; ModuleID = '/Users/cbieneman/dev/llvm-project/clang/test/SemaHLSL/ArrayTemporary.hlsl' |
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.
Is this .ll
file necessary? It doesn't look like a lit test file, no RUN
commands etc.
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.
IIUC, this file was added unintentionally, sent out #87346
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.
Doh! I'll delete it.
HLSL constant sized array function parameters do not decay to pointers.
Instead constant sized array types are preserved as unique types for
overload resolution, template instantiation and name mangling.
This implements the change by adding a new
ArrayParameterType
whichrepresents a non-decaying
ConstantArrayType
. The new type behaves thesame as
ConstantArrayType
except that it does not decay to a pointer.Values of
ConstantArrayType
in HLSL decay during overload resolutionvia a new
HLSLArrayRValue
cast toArrayParameterType
.ArrayParamterType
values are passed indirectly by-value to functionsin IR generation resulting in callee generated memcpy instructions.
The behavior of HLSL function calls is documented in the draft language specification under the Expr.Post.Call heading.
Additionally the design of this implementation approach is documented in Clang's documentation
Resolves #70123