Add length builtins and length HLSL function to DirectX Backend #101256
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-clang-codegen Author: Joshua Batista (bob80905) ChangesThis PR adds the length intrinsic and an HLSL function that uses it. Full diff: https://github.com/llvm/llvm-project/pull/101256.diff 13 Files Affected:
diff --git a/clang/docs/LanguageExtensions.rst b/clang/docs/LanguageExtensions.rst
index a747464582e77..45f081081a371 100644
--- a/clang/docs/LanguageExtensions.rst
+++ b/clang/docs/LanguageExtensions.rst
@@ -664,6 +664,7 @@ Unless specified otherwise operation(±0) = ±0 and operation(±infinity) = ±in
T __builtin_elementwise_cosh(T x) return the hyperbolic cosine of angle x in radians floating point types
T __builtin_elementwise_tanh(T x) return the hyperbolic tangent of angle x in radians floating point types
T __builtin_elementwise_floor(T x) return the largest integral value less than or equal to x floating point types
+ T __builtin_elementwise_length(T x) return the length of the specified floating-point vector floating point types
T __builtin_elementwise_log(T x) return the natural logarithm of x floating point types
T __builtin_elementwise_log2(T x) return the base 2 logarithm of x floating point types
T __builtin_elementwise_log10(T x) return the base 10 logarithm of x floating point types
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index dad44f45a847f..46f40889b4b33 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -243,6 +243,7 @@ DWARF Support in Clang
Floating Point Support in Clang
-------------------------------
+- Add ``__builtin_elementwise_length``builtin for floating point types only.
Fixed Point Support in Clang
----------------------------
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index 4133f6ff40cf3..0baadf0d196b2 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -4707,6 +4707,12 @@ def HLSLIsinf : LangBuiltin<"HLSL_LANG"> {
let Prototype = "void(...)";
}
+def HLSLLength : LangBuiltin<"HLSL_LANG"> {
+ let Spellings = ["__builtin_hlsl_elementwise_length"];
+ let Attributes = [NoThrow, Const];
+ let Prototype = "void(...)";
+}
+
def HLSLLerp : LangBuiltin<"HLSL_LANG"> {
let Spellings = ["__builtin_hlsl_lerp"];
let Attributes = [NoThrow, Const];
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index f0651c280ff95..a28073ca9ccc5 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -18460,6 +18460,22 @@ Value *CodeGenFunction::EmitHLSLBuiltinExpr(unsigned BuiltinID,
/*ReturnType=*/X->getType(), CGM.getHLSLRuntime().getLerpIntrinsic(),
ArrayRef<Value *>{X, Y, S}, nullptr, "hlsl.lerp");
}
+ case Builtin::BI__builtin_hlsl_elementwise_length: {
+ Value *X = EmitScalarExpr(E->getArg(0));
+
+ if (!E->getArg(0)->getType()->hasFloatingRepresentation())
+ llvm_unreachable("length operand must have a float representation");
+ // if the operand is a scalar, we can use the fabs llvm intrinsic directly
+ if (!E->getArg(0)->getType()->isVectorType()) {
+ llvm::Type *ResultType = ConvertType(E->getType());
+ Function *F = CGM.getIntrinsic(Intrinsic::fabs, ResultType);
+ return Builder.CreateCall(F, X);
+ }
+ return Builder.CreateIntrinsic(
+ /*ReturnType=*/X->getType()->getScalarType(),
+ CGM.getHLSLRuntime().getLengthIntrinsic(),
+ ArrayRef<Value *>{X}, nullptr, "hlsl.length");
+ }
case Builtin::BI__builtin_hlsl_elementwise_frac: {
Value *Op0 = EmitScalarExpr(E->getArg(0));
if (!E->getArg(0)->getType()->hasFloatingRepresentation())
diff --git a/clang/lib/CodeGen/CGHLSLRuntime.h b/clang/lib/CodeGen/CGHLSLRuntime.h
index 8c067f4963955..3f2dc0ae7b84d 100644
--- a/clang/lib/CodeGen/CGHLSLRuntime.h
+++ b/clang/lib/CodeGen/CGHLSLRuntime.h
@@ -75,6 +75,7 @@ class CGHLSLRuntime {
GENERATE_HLSL_INTRINSIC_FUNCTION(All, all)
GENERATE_HLSL_INTRINSIC_FUNCTION(Any, any)
GENERATE_HLSL_INTRINSIC_FUNCTION(Frac, frac)
+ GENERATE_HLSL_INTRINSIC_FUNCTION(Length, length)
GENERATE_HLSL_INTRINSIC_FUNCTION(Lerp, lerp)
GENERATE_HLSL_INTRINSIC_FUNCTION(Rsqrt, rsqrt)
GENERATE_HLSL_INTRINSIC_FUNCTION(ThreadId, thread_id)
diff --git a/clang/lib/Headers/hlsl/hlsl_intrinsics.h b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
index 09f26a4588c14..21ac25bba1acb 100644
--- a/clang/lib/Headers/hlsl/hlsl_intrinsics.h
+++ b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
@@ -908,6 +908,39 @@ float3 lerp(float3, float3, float3);
_HLSL_BUILTIN_ALIAS(__builtin_hlsl_lerp)
float4 lerp(float4, float4, float4);
+
+//===----------------------------------------------------------------------===//
+// length builtins
+//===----------------------------------------------------------------------===//
+
+/// \fn T length(T x)
+/// \brief Returns the length of the specified floating-point vector.
+/// \param x [in] The vector of floats, or a scalar float.
+///
+/// Length is based on the following formula: sqrt(x[0]^2 + x[1]^2 + �).
+
+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
+_HLSL_BUILTIN_ALIAS(__builtin_hlsl_elementwise_length)
+half length(half);
+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
+_HLSL_BUILTIN_ALIAS(__builtin_hlsl_elementwise_length)
+half length(half2);
+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
+_HLSL_BUILTIN_ALIAS(__builtin_hlsl_elementwise_length)
+half length(half3);
+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
+_HLSL_BUILTIN_ALIAS(__builtin_hlsl_elementwise_length)
+half length(half4);
+
+_HLSL_BUILTIN_ALIAS(__builtin_hlsl_elementwise_length)
+float length(float);
+_HLSL_BUILTIN_ALIAS(__builtin_hlsl_elementwise_length)
+float length(float2);
+_HLSL_BUILTIN_ALIAS(__builtin_hlsl_elementwise_length)
+float length(float3);
+_HLSL_BUILTIN_ALIAS(__builtin_hlsl_elementwise_length)
+float length(float4);
+
//===----------------------------------------------------------------------===//
// log builtins
//===----------------------------------------------------------------------===//
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index 9940bc5b4a606..624cbd3777bb8 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -1076,6 +1076,28 @@ bool SemaHLSL::CheckBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) {
return true;
break;
}
+ case Builtin::BI__builtin_hlsl_elementwise_length: {
+ if (SemaRef.checkArgCount(TheCall, 1))
+ return true;
+ if (SemaRef.PrepareBuiltinElementwiseMathOneArgCall(TheCall))
+ return true;
+
+ ExprResult A = TheCall->getArg(0);
+ QualType ArgTyA = A.get()->getType();
+ QualType RetTy;
+
+ if (auto *VTy = ArgTyA->getAs<VectorType>())
+ RetTy = VTy->getElementType();
+ else
+ RetTy = TheCall->getArg(0)->getType();
+
+ TheCall->setType(RetTy);
+
+
+ if (CheckFloatOrHalfRepresentations(&SemaRef, TheCall))
+ return true;
+ break;
+ }
case Builtin::BI__builtin_hlsl_mad: {
if (SemaRef.checkArgCount(TheCall, 3))
return true;
diff --git a/clang/test/CodeGenHLSL/builtins/length.hlsl b/clang/test/CodeGenHLSL/builtins/length.hlsl
new file mode 100644
index 0000000000000..0af669f36e6ba
--- /dev/null
+++ b/clang/test/CodeGenHLSL/builtins/length.hlsl
@@ -0,0 +1,73 @@
+// RUN: %clang_cc1 -finclude-default-header -x hlsl -triple \
+// RUN: dxil-pc-shadermodel6.3-library %s -fnative-half-type \
+// RUN: -emit-llvm -disable-llvm-passes -o - | FileCheck %s \
+// RUN: --check-prefixes=CHECK,NATIVE_HALF
+// RUN: %clang_cc1 -finclude-default-header -x hlsl -triple \
+// RUN: dxil-pc-shadermodel6.3-library %s -emit-llvm -disable-llvm-passes \
+// RUN: -o - | FileCheck %s --check-prefixes=CHECK,NO_HALF
+
+// NATIVE_HALF: define noundef half @
+// NATIVE_HALF: call half @llvm.fabs.f16(half
+// NO_HALF: call float @llvm.fabs.f32(float
+// NATIVE_HALF: ret half
+// NO_HALF: ret float
+half test_length_half(half p0)
+{
+ return length(p0);
+}
+// NATIVE_HALF: define noundef half @
+// NATIVE_HALF: %hlsl.length = call half @llvm.dx.length.v2f16
+// NO_HALF: %hlsl.length = call float @llvm.dx.length.v2f32(
+// NATIVE_HALF: ret half %hlsl.length
+// NO_HALF: ret float %hlsl.length
+half test_length_half2(half2 p0)
+{
+ return length(p0);
+}
+// NATIVE_HALF: define noundef half @
+// NATIVE_HALF: %hlsl.length = call half @llvm.dx.length.v3f16
+// NO_HALF: %hlsl.length = call float @llvm.dx.length.v3f32(
+// NATIVE_HALF: ret half %hlsl.length
+// NO_HALF: ret float %hlsl.length
+half test_length_half3(half3 p0)
+{
+ return length(p0);
+}
+// NATIVE_HALF: define noundef half @
+// NATIVE_HALF: %hlsl.length = call half @llvm.dx.length.v4f16
+// NO_HALF: %hlsl.length = call float @llvm.dx.length.v4f32(
+// NATIVE_HALF: ret half %hlsl.length
+// NO_HALF: ret float %hlsl.length
+half test_length_half4(half4 p0)
+{
+ return length(p0);
+}
+
+// CHECK: define noundef float @
+// CHECK: call float @llvm.fabs.f32(float
+// CHECK: ret float
+float test_length_float(float p0)
+{
+ return length(p0);
+}
+// CHECK: define noundef float @
+// CHECK: %hlsl.length = call float @llvm.dx.length.v2f32(
+// CHECK: ret float %hlsl.length
+float test_length_float2(float2 p0)
+{
+ return length(p0);
+}
+// CHECK: define noundef float @
+// CHECK: %hlsl.length = call float @llvm.dx.length.v3f32(
+// CHECK: ret float %hlsl.length
+float test_length_float3(float3 p0)
+{
+ return length(p0);
+}
+// CHECK: define noundef float @
+// CHECK: %hlsl.length = call float @llvm.dx.length.v4f32(
+// CHECK: ret float %hlsl.length
+float test_length_float4(float4 p0)
+{
+ return length(p0);
+}
\ No newline at end of file
diff --git a/clang/test/SemaHLSL/BuiltIns/length-errors.hlsl b/clang/test/SemaHLSL/BuiltIns/length-errors.hlsl
new file mode 100644
index 0000000000000..781c344f0da17
--- /dev/null
+++ b/clang/test/SemaHLSL/BuiltIns/length-errors.hlsl
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -finclude-default-header -triple dxil-pc-shadermodel6.6-library %s -fnative-half-type -emit-llvm -disable-llvm-passes -verify -verify-ignore-unexpected
+
+bool test_too_few_arg()
+{
+ return __builtin_hlsl_elementwise_length();
+ // expected-error@-1 {{too few arguments to function call, expected 1, have 0}}
+}
+
+bool2 test_too_many_arg(float2 p0)
+{
+ return __builtin_hlsl_elementwise_length(p0, p0);
+ // expected-error@-1 {{too many arguments to function call, expected 1, have 2}}
+}
+
+bool builtin_bool_to_float_type_promotion(bool p1)
+{
+ return __builtin_hlsl_elementwise_length(p1);
+ // expected-error@-1 {passing 'bool' to parameter of incompatible type 'float'}}
+}
+
+bool builtin_length_int_to_float_promotion(int p1)
+{
+ return __builtin_hlsl_elementwise_length(p1);
+ // expected-error@-1 {{passing 'int' to parameter of incompatible type 'float'}}
+}
+
+bool2 builtin_length_int2_to_float2_promotion(int2 p1)
+{
+ return __builtin_hlsl_elementwise_length(p1);
+ // expected-error@-1 {{passing 'int2' (aka 'vector<int, 2>') to parameter of incompatible type '__attribute__((__vector_size__(2 * sizeof(float)))) float' (vector of 2 'float' values)}}
+}
\ No newline at end of file
diff --git a/llvm/include/llvm/IR/IntrinsicsDirectX.td b/llvm/include/llvm/IR/IntrinsicsDirectX.td
index a7f212da2f5b6..47c01f899a926 100644
--- a/llvm/include/llvm/IR/IntrinsicsDirectX.td
+++ b/llvm/include/llvm/IR/IntrinsicsDirectX.td
@@ -55,6 +55,8 @@ def int_dx_isinf :
def int_dx_lerp : Intrinsic<[LLVMMatchType<0>], [llvm_anyfloat_ty, LLVMMatchType<0>,LLVMMatchType<0>],
[IntrNoMem, IntrWillReturn] >;
+def int_dx_length : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], [llvm_anyfloat_ty]>;
+
def int_dx_imad : DefaultAttrsIntrinsic<[llvm_anyint_ty], [LLVMMatchType<0>, LLVMMatchType<0>, LLVMMatchType<0>]>;
def int_dx_umad : DefaultAttrsIntrinsic<[llvm_anyint_ty], [LLVMMatchType<0>, LLVMMatchType<0>, LLVMMatchType<0>]>;
def int_dx_rcp : DefaultAttrsIntrinsic<[llvm_anyfloat_ty], [LLVMMatchType<0>]>;
diff --git a/llvm/include/llvm/IR/IntrinsicsSPIRV.td b/llvm/include/llvm/IR/IntrinsicsSPIRV.td
index ef6ddf12c32f6..c91fe859d7cc2 100644
--- a/llvm/include/llvm/IR/IntrinsicsSPIRV.td
+++ b/llvm/include/llvm/IR/IntrinsicsSPIRV.td
@@ -63,5 +63,7 @@ let TargetPrefix = "spv" in {
def int_spv_frac : DefaultAttrsIntrinsic<[LLVMMatchType<0>], [llvm_anyfloat_ty]>;
def int_spv_lerp : Intrinsic<[LLVMMatchType<0>], [llvm_anyfloat_ty, LLVMMatchType<0>,LLVMMatchType<0>],
[IntrNoMem, IntrWillReturn] >;
+ def int_spv_length : Intrinsic<[LLVMMatchType<0>], [llvm_anyfloat_ty, LLVMMatchType<0>,LLVMMatchType<0>],
+ [IntrNoMem, IntrWillReturn] >;
def int_spv_rsqrt : DefaultAttrsIntrinsic<[LLVMMatchType<0>], [llvm_anyfloat_ty]>;
}
diff --git a/llvm/lib/Target/DirectX/DXILIntrinsicExpansion.cpp b/llvm/lib/Target/DirectX/DXILIntrinsicExpansion.cpp
index 4b162a35365c8..7ef5b9eae9310 100644
--- a/llvm/lib/Target/DirectX/DXILIntrinsicExpansion.cpp
+++ b/llvm/lib/Target/DirectX/DXILIntrinsicExpansion.cpp
@@ -42,6 +42,7 @@ static bool isIntrinsicExpansion(Function &F) {
case Intrinsic::dx_clamp:
case Intrinsic::dx_uclamp:
case Intrinsic::dx_lerp:
+ case Intrinsic::dx_length:
case Intrinsic::dx_sdot:
case Intrinsic::dx_udot:
return true;
@@ -157,6 +158,42 @@ static bool expandAnyIntrinsic(CallInst *Orig) {
return true;
}
+static bool expandLengthIntrinsic(CallInst *Orig) {
+ Value *X = Orig->getOperand(0);
+ IRBuilder<> Builder(Orig->getParent());
+ Builder.SetInsertPoint(Orig);
+ Type *Ty = X->getType();
+ Type *EltTy = Ty->getScalarType();
+
+ // Though dx.length does work on scalar type, we can optimize it to just emit
+ // fabs, in CGBuiltin.cpp. We shouldn't see a scalar type here because
+ // CGBuiltin.cpp should have emitted a fabs call.
+ assert(Ty->isVectorTy() && "dx.length only works on vector type");
+ Value *Elt = Builder.CreateExtractElement(X, (uint64_t)0);
+ auto *XVec = dyn_cast<FixedVectorType>(Ty);
+ unsigned size = XVec->getNumElements();
+ if (size > 1) {
+ Value *Sum = Builder.CreateFMul(Elt, Elt);
+ for (unsigned i = 1; i < size; i++) {
+ Elt = Builder.CreateExtractElement(X, i);
+ Value *Mul = Builder.CreateFMul(Elt, Elt);
+ Sum = Builder.CreateFAdd(Sum, Mul);
+ }
+ Value *Result = Builder.CreateIntrinsic(
+ EltTy, Intrinsic::sqrt, ArrayRef<Value *>{Sum}, nullptr, "elt.sqrt");
+
+ Orig->replaceAllUsesWith(Result);
+ Orig->eraseFromParent();
+ return true;
+ } else {
+ Value *Result = Builder.CreateIntrinsic(
+ EltTy, Intrinsic::fabs, ArrayRef<Value *>{Elt}, nullptr, "elt.abs");
+ Orig->replaceAllUsesWith(Result);
+ Orig->eraseFromParent();
+ return true;
+ }
+}
+
static bool expandLerpIntrinsic(CallInst *Orig) {
Value *X = Orig->getOperand(0);
Value *Y = Orig->getOperand(1);
@@ -280,6 +317,8 @@ static bool expandIntrinsic(Function &F, CallInst *Orig) {
return expandClampIntrinsic(Orig, F.getIntrinsicID());
case Intrinsic::dx_lerp:
return expandLerpIntrinsic(Orig);
+ case Intrinsic::dx_length:
+ return expandLengthIntrinsic(Orig);
case Intrinsic::dx_sdot:
case Intrinsic::dx_udot:
return expandIntegerDot(Orig, F.getIntrinsicID());
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
index 8391e0dec9a39..0f0b7fee96559 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
@@ -280,7 +280,7 @@ void SPIRVInstructionSelector::setupMF(MachineFunction &MF, GISelKnownBits *KB,
CodeGenCoverage *CoverageInfo,
ProfileSummaryInfo *PSI,
BlockFrequencyInfo *BFI) {
- MMI = &MF.getMMI().getObjFileInfo<SPIRVMachineModuleInfo>();
+ // MMI = &MF.getMMI().getObjFileInfo<SPIRVMachineModuleInfo>();
MRI = &MF.getRegInfo();
GR.setCurrentFunc(MF);
InstructionSelector::setupMF(MF, KB, CoverageInfo, PSI, BFI);
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
farzonl
reviewed
Jul 30, 2024
farzonl
reviewed
Jul 30, 2024
farzonl
reviewed
Jul 30, 2024
farzonl
reviewed
Jul 30, 2024
farzonl
reviewed
Jul 30, 2024
farzonl
reviewed
Jul 30, 2024
farzonl
reviewed
Jul 30, 2024
farzonl
reviewed
Jul 30, 2024
farzonl
reviewed
Jul 30, 2024
farzonl
reviewed
Jul 30, 2024
farzonl
reviewed
Jul 30, 2024
farzonl
reviewed
Jul 30, 2024
farzonl
requested changes
Jul 30, 2024
There was a problem hiding this comment.
You need test cases for the DXIL expansion pass and the dxil-op-lower pass. It can be one file called llvm/test/CodeGen/DirectX/length.ll
It should start off like:
; RUN: opt -S -dxil-intrinsic-expansion < %s | FileCheck %s --check-prefixes=CHECK,EXPCHECK
; RUN: opt -S -dxil-op-lower < %s | FileCheck %s --check-prefixes=CHECK,DOPCHECK
If you want an example see llvm/test/CodeGen/DirectX/idot.ll
You don't want to mark this as fixed because it will close out this ticket and you still need to address the SPIRV backend changes. |
farzonl
reviewed
Jul 31, 2024
farzonl
reviewed
Jul 31, 2024
farzonl
reviewed
Jul 31, 2024
farzonl
approved these changes
Jul 31, 2024
|
Nice work LGTM! |
damyanp
reviewed
Aug 1, 2024
| @@ -63,5 +63,6 @@ let TargetPrefix = "spv" in { | |||
| def int_spv_frac : DefaultAttrsIntrinsic<[LLVMMatchType<0>], [llvm_anyfloat_ty]>; | |||
| def int_spv_lerp : Intrinsic<[LLVMMatchType<0>], [llvm_anyfloat_ty, LLVMMatchType<0>,LLVMMatchType<0>], | |||
| [IntrNoMem, IntrWillReturn] >; | |||
| def int_spv_length : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], [llvm_anyfloat_ty]>; | |||
There was a problem hiding this comment.
I thought this change didn't include the SPIRV parts of this?
There was a problem hiding this comment.
He has to define this because of CGM.getHLSLRuntime().getLengthIntrinsic(). the alternative would be to just emit int_dx_length in CGBuiltin.cpp until there is a spirv implementation. then swap in getLengthIntrinsic
damyanp
reviewed
Aug 1, 2024
| @@ -908,6 +908,38 @@ float3 lerp(float3, float3, float3); | |||
| _HLSL_BUILTIN_ALIAS(__builtin_hlsl_lerp) | |||
| float4 lerp(float4, float4, float4); | |||
|
|
|||
| //===----------------------------------------------------------------------===// | |||
There was a problem hiding this comment.
PR title looks like it may be misnamed, since this is also adding the HLSL builtin functions, it is doing more than just adding it to the backend.
bogner
reviewed
Aug 1, 2024
| @@ -4707,6 +4707,12 @@ def HLSLIsinf : LangBuiltin<"HLSL_LANG"> { | |||
| let Prototype = "void(...)"; | |||
| } | |||
|
|
|||
| def HLSLLength : LangBuiltin<"HLSL_LANG"> { | |||
| let Spellings = ["__builtin_hlsl_elementwise_length"]; | |||
There was a problem hiding this comment.
I think this should be "__builtin_hlsl_length", not "__builtin_hlsl_elementwise_length". The word "elementwise" means we do an operation to each element, whereas this is an operation on the whole vector.
There was a problem hiding this comment.
I agree with Justin, This shouldn't be an elementwise builtin see lerp, dot, and mad. For operations that work on vectors and generate scalar results we don't add the elementwise naming convention. Reserve elementwise for cases were the function takes multiple inputs applys the same operation to each input then returns the same number of outputs.
| Value *Elt = Builder.CreateExtractElement(X, (uint64_t)0); | ||
| auto *XVec = dyn_cast<FixedVectorType>(Ty); | ||
| unsigned size = XVec->getNumElements(); | ||
| assert(Ty->isVectorTy() && size > 1 && "dx.length only works on vector type"); |
There was a problem hiding this comment.
The message should probably say "dx.length requires a vector of length 2 or more"
| // CGBuiltin.cpp should have emitted a fabs call. | ||
| Value *Elt = Builder.CreateExtractElement(X, (uint64_t)0); | ||
| auto *XVec = dyn_cast<FixedVectorType>(Ty); | ||
| unsigned size = XVec->getNumElements(); |
llvm/test/CodeGen/DirectX/length.ll
Outdated
Comment on lines
16
to
22
| define noundef half @test_length_half(half noundef %p0) { | ||
| entry: | ||
| ; EXPCHECK: call half @llvm.fabs.f16(half %{{.*}}) | ||
| ; DOPCHECK: call half @dx.op.unary.f16(i32 6, half %{{.*}}) | ||
| %0 = call half @llvm.fabs.f16(half %p0) | ||
| ret half %0 | ||
| } |
There was a problem hiding this comment.
This test probably doesn't need to be here, given that we have separate tests for fabs
| entry: | ||
| %hlsl.length = call double @llvm.dx.length.v2f32(<2 x double> %p0) | ||
| ret double %hlsl.length | ||
| } |
There was a problem hiding this comment.
Do we also need a test that we error on dx.length of a scalar?
There was a problem hiding this comment.
Wouldn't that mean changing assert(Ty->isVectorTy() && size > 1); to an llvm error?
There was a problem hiding this comment.
I beleive it would. Is that a problem?
Also, I should note, the dot* intrinsics also expect vector arguments, and don't test for scalar inputs.
There was a problem hiding this comment.
Isn't it already an error? The intrinsic is defined in a way that requires a vector:
def int_dx_length : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], [llvm_anyfloat_ty]>;
That said I guess there is an argument that the crash on a vector of length 1 isn't ideal. Maybe we should make that an error as well.
bob80905
changed the title
Co-authored-by: Justin Bogner <[email protected]>
Co-authored-by: Justin Bogner <[email protected]>
bogner
approved these changes
Aug 2, 2024
farzonl
reviewed
Aug 2, 2024
|
|
||
| define noundef float @test_length_float(float noundef %p0) { | ||
| entry: | ||
| %hlsl.length = call float @llvm.dx.length.f32(float %p0) |
There was a problem hiding this comment.
We get this error for free from how we define the intrinsic. I thought what we wanted an error test for was @llvm.dx.length.v1f32?
There was a problem hiding this comment.
Ok, I've added tests for both a scalar input, and a vector with 1 element input.
MaskRay
added a commit
that referenced
this pull request
Aug 5, 2024
bob80905
added a commit
that referenced
this pull request
Aug 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds the length intrinsic and an HLSL function that uses it.
The SPIRV implementation is left for a future PR.
This PR addresses #99134, though some SPIR-V changes still need to be made to complete the task. Below is how this PR addresses #99134.
lengthclang builtin" was done by definingHLSL Lengthin Builtins.tdlengthclang builtin with hlsl_intrinsics.h" was done by using the alias attribute to makelengthan alias of__builtin_hlsl_elementwise_lengthin hlsl_intrinsics.hlengthtoCheckHLSLBuiltinFunctionCallinSemaChecking.cpp" was done, but in this case not in SemaChecking.cpp, rather SemaHLSL.cpp. A case was added to the builtin to check for semantic failures, and setTheCallup to have the right return type.lengthtoEmitHLSLBuiltinExprinCGBuiltin.cpp" was done. For scalars, fabs is emitted, otherwise, length is emitted.clang/test/CodeGenHLSL/builtins/length.hlslwas done to test thatlengthin HLSL emits the right intrinsic.clang/test/SemaHLSL/BuiltIns/length-errors.hlsl" was done to test for diagnostics emitted in SemaHLSL.cppint_dx_lengthintrinsic inIntrinsicsDirectX.td" was done. Specifying return types and parameter types was difficult, butidotwas used for reference, andllvm\include\llvm\IR\Intrinsics.tdcontains all the ways to express return / parameter types.int_dx_lengthinllvm/lib/Target/DirectX/DXILIntrinsicExpansion.cpp" was done, and was mostly derived by looking atTranslateLengthinHLOperationLower.cppin the DXC codebase.length.llandlength_errors.lltests inllvm/test/CodeGen/DirectX/" was done by taking the DXIL output ofclang/test/CodeGenHLSL/builtins/length.hlsland runningopt -S -dxil-intrinsic-expansionandopt -S -dxil-op-loweron it, checking for how the length intrinsic was either expanded or lowered.int_spv_lengthintrinsic inIntrinsicsSPIRV.td" was done by copyingIntrinsicsDirectX.td.