-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[HLSL] Vector standard conversions #71098
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
Changes from 6 commits
91e8d9d
06c7a72
6cbf74c
899d529
86c0dc0
ac9711e
f86c51f
b958512
f2c4024
2b90d41
c5430b3
20a095e
003bd4b
f1c13db
34ad305
fdd6a97
ef34f7e
9982104
b2b0655
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -610,6 +610,8 @@ class ScalarExprEmitter | |||||
llvm::Value *EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV, | ||||||
bool isInc, bool isPre); | ||||||
|
||||||
llvm::Value *EmitVectorElementConversion(QualType SrcType, QualType DstType, | ||||||
llvm::Value *Src); | ||||||
|
||||||
Value *VisitUnaryAddrOf(const UnaryOperator *E) { | ||||||
if (isa<MemberPointerType>(E->getType())) // never sugared | ||||||
|
@@ -1422,6 +1424,9 @@ Value *ScalarExprEmitter::EmitScalarConversion(Value *Src, QualType SrcType, | |||||
return Builder.CreateVectorSplat(NumElements, Src, "splat"); | ||||||
} | ||||||
|
||||||
if (SrcType->isExtVectorType() && DstType->isExtVectorType()) | ||||||
return EmitVectorElementConversion(SrcType, DstType, Src); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EmitScalarConversion is a mess; we have cast kinds, but we never taught this part of CodeGen to use them, so it just guesses the cast kind based on the source and destination types. This is particularly bad for vectors, since it's often possible to cast them in multiple different ways; I'm not confident this change won't have unintended side-effects. Please fix the code for the relevant cast kinds to avoid going through EmitScalarConversion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I believe the reason we still have this whole analysis in |
||||||
|
||||||
if (SrcType->isMatrixType() && DstType->isMatrixType()) | ||||||
return EmitScalarCast(Src, SrcType, DstType, SrcTy, DstTy, Opts); | ||||||
|
||||||
|
@@ -1701,10 +1706,14 @@ Value *ScalarExprEmitter::VisitShuffleVectorExpr(ShuffleVectorExpr *E) { | |||||
} | ||||||
|
||||||
Value *ScalarExprEmitter::VisitConvertVectorExpr(ConvertVectorExpr *E) { | ||||||
QualType SrcType = E->getSrcExpr()->getType(), | ||||||
DstType = E->getType(); | ||||||
QualType SrcType = E->getSrcExpr()->getType(), DstType = E->getType(); | ||||||
Value *Src = CGF.EmitScalarExpr(E->getSrcExpr()); | ||||||
return EmitVectorElementConversion(SrcType, DstType, Src); | ||||||
} | ||||||
|
||||||
Value *Src = CGF.EmitScalarExpr(E->getSrcExpr()); | ||||||
llvm::Value *ScalarExprEmitter::EmitVectorElementConversion(QualType SrcType, | ||||||
QualType DstType, | ||||||
Value *Src) { | ||||||
|
||||||
SrcType = CGF.getContext().getCanonicalType(SrcType); | ||||||
DstType = CGF.getContext().getCanonicalType(DstType); | ||||||
|
@@ -2466,6 +2475,14 @@ Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) { | |||||
case CK_IntToOCLSampler: | ||||||
return CGF.CGM.createOpenCLIntToSamplerConversion(E, CGF); | ||||||
|
||||||
case CK_HLSLVectorTruncation: { | ||||||
assert(DestTy->isVectorType() && "Expected dest type to be vector type"); | ||||||
Value *Vec = Visit(const_cast<Expr *>(E)); | ||||||
SmallVector<int, 16> Mask; | ||||||
Mask.insert(Mask.begin(), DestTy->getAs<VectorType>()->getNumElements(), 0); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return Builder.CreateShuffleVector(Vec, Mask, "trunc"); | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, please tell me this is wrong. The result of truncating There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doh! |
||||||
|
||||||
} // end of switch | ||||||
|
||||||
llvm_unreachable("unknown scalar cast"); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15433,11 +15433,18 @@ static void CheckImplicitConversion(Sema &S, Expr *E, QualType T, | |
if (S.SourceMgr.isInSystemMacro(CC)) | ||
return; | ||
return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_vector_scalar); | ||
} else if (S.getLangOpts().HLSL && | ||
Target->getAs<VectorType>()->getNumElements() < | ||
Source->getAs<VectorType>()->getNumElements()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use |
||
// Diagnose vector truncation but don't return. We may also want to | ||
// diagnose an element conversion. | ||
DiagnoseImpCast(S, E, T, CC, diag::warn_hlsl_impcast_vector_truncation); | ||
} | ||
|
||
// If the vector cast is cast between two vectors of the same size, it is | ||
// a bitcast, not a conversion. | ||
if (S.Context.getTypeSize(Source) == S.Context.getTypeSize(Target)) | ||
// a bitcast, not a conversion, except under HLSL where it is a conversion. | ||
if (!S.getLangOpts().HLSL && | ||
S.Context.getTypeSize(Source) == S.Context.getTypeSize(Target)) | ||
return; | ||
|
||
Source = cast<VectorType>(Source)->getElementType().getTypePtr(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4763,6 +4763,20 @@ Sema::PerformImplicitConversion(Expr *From, QualType ToType, | |
CK_ZeroToOCLOpaqueType, | ||
From->getValueKind()).get(); | ||
break; | ||
case ICK_HLSL_Vector_Truncation: { | ||
// Note: HLSL vectors are ExtVectors. Since this truncates a vector to a | ||
// smaller vector, this can only operate on arguments where the source and | ||
// destination types are ExtVectors. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you actually forbid the creation of non-ExtVector types with attributes, or is it just that HLSL vector syntax normally makes ExtVectors? Because I don't think your conversion handling code makes a distinction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HLSL's vector syntax produces ExtVectors. The template<typename T, int Sz>
using vector = T __attribute__((ext_vector_type(Sz))); Then all the explicit types are just typedefs off that defined in We haven't explicitly disabled non-Ext vector types in Clang, but they are disabled in DXC. I'm unsure one way or another whether we should explicitly disable them in Clang for HLSL. |
||
auto *FromVec = From->getType()->castAs<ExtVectorType>(); | ||
auto *ToVec = ToType->castAs<ExtVectorType>(); | ||
QualType ElType = FromVec->getElementType(); | ||
QualType TruncTy = | ||
Context.getExtVectorType(ElType, ToVec->getNumElements()); | ||
From = ImpCastExprToType(From, TruncTy, CK_HLSLVectorTruncation, | ||
From->getValueKind()) | ||
.get(); | ||
break; | ||
} | ||
|
||
case ICK_Lvalue_To_Rvalue: | ||
case ICK_Array_To_Pointer: | ||
|
@@ -4775,6 +4789,76 @@ Sema::PerformImplicitConversion(Expr *From, QualType ToType, | |
llvm_unreachable("Improper second standard conversion"); | ||
} | ||
|
||
if (SCS.Element != ICK_Identity) { | ||
// If SCS.Element is not ICK_Identity the To and From types must be HLSL | ||
// vectors or matrices. HLSL matrices aren't yet supported so this code only | ||
// handles vectors for now. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment about matrices should probably be a |
||
|
||
assert(From->getType()->isVectorType() && ToType->isVectorType() && | ||
"Element conversion is only supported for vector types."); | ||
assert(From->getType()->getAs<VectorType>()->getNumElements() == | ||
ToType->getAs<VectorType>()->getNumElements() && | ||
"Element conversion is only supported for vectors with the same " | ||
"element counts."); | ||
QualType FromElTy = From->getType()->getAs<VectorType>()->getElementType(); | ||
unsigned NumElts = ToType->getAs<VectorType>()->getNumElements(); | ||
switch (SCS.Element) { | ||
case ICK_Identity: | ||
// Nothing to do. | ||
break; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
case ICK_Boolean_Conversion: | ||
// Perform half-to-boolean conversion via float. | ||
if (FromElTy->isHalfType()) { | ||
QualType FPExtType = Context.getExtVectorType(FromElTy, NumElts); | ||
From = ImpCastExprToType(From, FPExtType, CK_FloatingCast).get(); | ||
FromType = FPExtType; | ||
} | ||
|
||
From = | ||
ImpCastExprToType(From, ToType, ScalarTypeToBooleanCastKind(FromElTy), | ||
VK_PRValue, | ||
/*BasePath=*/nullptr, CCK) | ||
.get(); | ||
break; | ||
case ICK_Integral_Promotion: | ||
case ICK_Integral_Conversion: | ||
if (ToType->isBooleanType()) { | ||
assert(FromType->castAs<EnumType>()->getDecl()->isFixed() && | ||
SCS.Second == ICK_Integral_Promotion && | ||
"only enums with fixed underlying type can promote to bool"); | ||
From = ImpCastExprToType(From, ToType, CK_IntegralToBoolean, VK_PRValue, | ||
/*BasePath=*/nullptr, CCK) | ||
.get(); | ||
} else { | ||
From = ImpCastExprToType(From, ToType, CK_IntegralCast, VK_PRValue, | ||
/*BasePath=*/nullptr, CCK) | ||
.get(); | ||
} | ||
break; | ||
|
||
case ICK_Floating_Promotion: | ||
case ICK_Floating_Conversion: | ||
From = ImpCastExprToType(From, ToType, CK_FloatingCast, VK_PRValue, | ||
/*BasePath=*/nullptr, CCK) | ||
.get(); | ||
break; | ||
case ICK_Floating_Integral: | ||
if (ToType->isRealFloatingType()) | ||
From = | ||
ImpCastExprToType(From, ToType, CK_IntegralToFloating, VK_PRValue, | ||
/*BasePath=*/nullptr, CCK) | ||
.get(); | ||
else | ||
From = | ||
ImpCastExprToType(From, ToType, CK_FloatingToIntegral, VK_PRValue, | ||
/*BasePath=*/nullptr, CCK) | ||
.get(); | ||
break; | ||
default: | ||
llvm_unreachable("Improper element standard conversion"); | ||
} | ||
} | ||
|
||
switch (SCS.Third) { | ||
case ICK_Identity: | ||
// Nothing to do. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6427,7 +6427,7 @@ void InitializationSequence::InitializeFrom(Sema &S, | |
// For HLSL ext vector types we allow list initialization behavior for C++ | ||
// constructor syntax. This is accomplished by converting initialization | ||
// arguments an InitListExpr late. | ||
if (S.getLangOpts().HLSL && DestType->isExtVectorType() && | ||
if (S.getLangOpts().HLSL && Args.size() > 1 && DestType->isExtVectorType() && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sort of looks like a separate bug fix. In any case I don't see a test for it (though maybe I missed it) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is subtle but there are tests that cover this. The tests introduced here cover the void Fn(double2 D);
void Call(float2 F) {
Fn(F);
} In the call to In other cases that we cover in the vector-constructors.hlsl test, we initialize vectors with initializer lists that are more than one argument like: float2 f = float2(1, 2); |
||
(SourceType.isNull() || | ||
!Context.hasSameUnqualifiedType(SourceType, DestType))) { | ||
|
||
|
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.
Unfortunately, we already use "truncate" a lot to talk about narrowing integer conversions. Ideally, the term here wouldn't be confusable with that; I don't have a great alternative to suggest, though. Even if we can't find a better name, though, please briefly describe the conversion here, since it's not obvious from context.
As an aside: uh, wow, that sure seems like an unfortunate conversion to have in the language. I guess it's not that bad for something like dropping the last element from a 4-vector, where often it's really a 3-vector in disguise. Still, oof, what a footgun.
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.
Yea... HLSL has a lot of unfortunate implicit language behaviors. Part of our goal with the Clang implementation is to capture those behaviors in a way we can diagnose them more accurately, and slowly tighten up the language as we evolve it.
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.
Okay. I think this patch overall looks fine to land, except please do expand on the comment here to explain what truncating a vector type means (dropping elements from the end).