Skip to content

[spirv] Fix firstbithigh/low signedness #5665

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

Merged
merged 1 commit into from
Sep 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 32 additions & 22 deletions tools/clang/lib/SPIRV/SpirvEmitter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8313,25 +8313,6 @@ SpirvEmitter::processIntrinsicCallExpr(const CallExpr *callExpr) {
srcLoc, srcRange); \
} break

#define INTRINSIC_OP_CASE_SINT_UINT(intrinsicOp, glslSintOp, glslUintOp, \
doEachVec) \
case hlsl::IntrinsicOp::IOP_##intrinsicOp: { \
glslOpcode = isSintType ? GLSLstd450::GLSLstd450##glslSintOp \
: GLSLstd450::GLSLstd450##glslUintOp; \
retVal = processIntrinsicUsingGLSLInst(callExpr, glslOpcode, doEachVec, \
srcLoc, srcRange); \
} break

#define INTRINSIC_OP_CASE_SINT_UINT_FLOAT(intrinsicOp, glslSintOp, glslUintOp, \
glslFloatOp, doEachVec) \
case hlsl::IntrinsicOp::IOP_##intrinsicOp: { \
glslOpcode = isFloatType ? GLSLstd450::GLSLstd450##glslFloatOp \
: isSintType ? GLSLstd450::GLSLstd450##glslSintOp \
: GLSLstd450::GLSLstd450##glslUintOp; \
retVal = processIntrinsicUsingGLSLInst(callExpr, glslOpcode, doEachVec, \
srcLoc, srcRange); \
} break

switch (const auto hlslOpcode = static_cast<hlsl::IntrinsicOp>(opcode)) {
case hlsl::IntrinsicOp::IOP_InterlockedAdd:
case hlsl::IntrinsicOp::IOP_InterlockedAnd:
Expand Down Expand Up @@ -8741,6 +8722,18 @@ SpirvEmitter::processIntrinsicCallExpr(const CallExpr *callExpr) {
retVal = processIntrinsicUsingGLSLInst(callExpr, glslOpcode, true, srcLoc,
srcRange);
break;
}
case hlsl::IntrinsicOp::IOP_ufirstbithigh: {
retVal = processIntrinsicFirstbit(callExpr, GLSLstd450::GLSLstd450FindUMsb);
break;
}
case hlsl::IntrinsicOp::IOP_firstbithigh: {
retVal = processIntrinsicFirstbit(callExpr, GLSLstd450::GLSLstd450FindSMsb);
break;
}
case hlsl::IntrinsicOp::IOP_firstbitlow: {
retVal = processIntrinsicFirstbit(callExpr, GLSLstd450::GLSLstd450FindILsb);
break;
}
INTRINSIC_SPIRV_OP_CASE(ddx, DPdx, true);
INTRINSIC_SPIRV_OP_CASE(ddx_coarse, DPdxCoarse, false);
Expand Down Expand Up @@ -8772,10 +8765,7 @@ SpirvEmitter::processIntrinsicCallExpr(const CallExpr *callExpr) {
INTRINSIC_OP_CASE(determinant, Determinant, false);
INTRINSIC_OP_CASE(exp, Exp, true);
INTRINSIC_OP_CASE(exp2, Exp2, true);
INTRINSIC_OP_CASE_SINT_UINT(firstbithigh, FindSMsb, FindUMsb, false);
INTRINSIC_OP_CASE_SINT_UINT(ufirstbithigh, FindSMsb, FindUMsb, false);
INTRINSIC_OP_CASE(faceforward, FaceForward, false);
INTRINSIC_OP_CASE(firstbitlow, FindILsb, false);
INTRINSIC_OP_CASE(floor, Floor, true);
INTRINSIC_OP_CASE(fma, Fma, true);
INTRINSIC_OP_CASE(frac, Fract, true);
Expand Down Expand Up @@ -8813,6 +8803,26 @@ SpirvEmitter::processIntrinsicCallExpr(const CallExpr *callExpr) {
return retVal;
}

SpirvInstruction *
SpirvEmitter::processIntrinsicFirstbit(const CallExpr *callExpr,
GLSLstd450 glslOpcode) {
const FunctionDecl *callee = callExpr->getDirectCallee();
const SourceLocation srcLoc = callExpr->getExprLoc();
const SourceRange srcRange = callExpr->getSourceRange();
const QualType argType = callExpr->getArg(0)->getType();

if (astContext.getTypeSize(argType) == 64) {
emitError("%0 is not yet implemented for 64-bit width components when "
"targetting SPIR-V",
srcLoc)
<< getFunctionOrOperatorName(callee, true);
return nullptr;
}

return processIntrinsicUsingGLSLInst(callExpr, glslOpcode, false, srcLoc,
srcRange);
}

SpirvInstruction *
SpirvEmitter::processIntrinsicInterlockedMethod(const CallExpr *expr,
hlsl::IntrinsicOp opcode) {
Expand Down
4 changes: 4 additions & 0 deletions tools/clang/lib/SPIRV/SpirvEmitter.h
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,10 @@ class SpirvEmitter : public ASTConsumer {
SpirvInstruction *processIntrinsicExecutionMode(const CallExpr *expr,
bool useIdParams);

/// Processes the 'firstbit{high|low}' intrinsic functions.
SpirvInstruction *processIntrinsicFirstbit(const CallExpr *,
GLSLstd450 glslOpcode);

private:
/// Returns the <result-id> for constant value 0 of the given type.
SpirvConstant *getValueZero(QualType type);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// RUN: %dxc -T ps_6_0 -E main

void main() {
uint64_t uint_1;
int fbh = firstbithigh(uint_1);
}

// CHECK: error: firstbithigh is not yet implemented for 64-bit width components when targetting SPIR-V
23 changes: 14 additions & 9 deletions tools/clang/test/CodeGenSPIRV/intrinsics.firstbithigh.hlsl
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
// RUN: %dxc -T ps_6_0 -E main

// Note: Even though the HLSL documentation contains a version of "firstbithigh" that
// takes signed integer(s) and returns signed integer(s), the frontend always generates
// the AST using the overloaded version that takes unsigned integer(s) and returns
// unsigned integer(s). Therefore "FindSMsb" is not generated in any case below.

// CHECK: [[glsl:%\d+]] = OpExtInstImport "GLSL.std.450"

void main() {
Expand All @@ -13,15 +8,25 @@ void main() {
uint uint_1;
uint4 uint_4;

// CHECK: {{%\d+}} = OpExtInst %uint [[glsl]] FindUMsb {{%\d+}}
// CHECK: [[sint_1:%\d+]] = OpLoad %int %sint_1
// CHECK: [[msb:%\d+]] = OpExtInst %uint [[glsl]] FindSMsb [[sint_1]]
// CHECK: [[res:%\d+]] = OpBitcast %int [[msb]]
// CHECK: OpStore %fbh [[res]]
int fbh = firstbithigh(sint_1);

// CHECK: {{%\d+}} = OpExtInst %v4uint [[glsl]] FindUMsb {{%\d+}}
// CHECK: [[sint_4:%\d+]] = OpLoad %v4int %sint_4
// CHECK: [[msb:%\d+]] = OpExtInst %v4uint [[glsl]] FindSMsb [[sint_4]]
// CHECK: [[res:%\d+]] = OpBitcast %v4int [[msb]]
// CHECK: OpStore %fbh4 [[res]]
int4 fbh4 = firstbithigh(sint_4);

// CHECK: {{%\d+}} = OpExtInst %uint [[glsl]] FindUMsb {{%\d+}}
// CHECK: [[uint_1:%\d+]] = OpLoad %uint %uint_1
// CHECK: [[msb:%\d+]] = OpExtInst %uint [[glsl]] FindUMsb [[uint_1]]
// CHECK: OpStore %ufbh [[msb]]
uint ufbh = firstbithigh(uint_1);

// CHECK: {{%\d+}} = OpExtInst %v4uint [[glsl]] FindUMsb {{%\d+}}
// CHECK: [[uint_4:%\d+]] = OpLoad %v4uint %uint_4
// CHECK: [[msb:%\d+]] = OpExtInst %v4uint [[glsl]] FindUMsb [[uint_4]]
// CHECK: OpStore %ufbh4 [[msb]]
uint4 ufbh4 = firstbithigh(uint_4);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// RUN: %dxc -T ps_6_0 -E main

void main() {
uint64_t uint_1;
int fbl = firstbitlow(uint_1);
}

// CHECK: error: firstbitlow is not yet implemented for 64-bit width components when targetting SPIR-V
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ void main() {
// CHECK-NEXT: DebugLine [[src]] %uint_180 %uint_180 %uint_20 %uint_43
// CHECK-NEXT: OpExtInst %float {{%[0-9]+}} Sqrt [[abs]]
// CHECK: DebugLine [[src]] %uint_180 %uint_180 %uint_7 %uint_52
// CHECK-NEXT: OpExtInst %uint {{%[0-9]+}} FindUMsb
// CHECK-NEXT: OpExtInst %uint {{%[0-9]+}} FindSMsb
max(firstbithigh(sqrt(abs(v2f.x * v4f.w)) + v4i.x),
// CHECK: DebugLine [[src]] %uint_183 %uint_183 %uint_7 %uint_16
// CHECK-NEXT: OpExtInst %float {{%[0-9]+}} Cos
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ void main() {
// CHECK-NEXT: OpLine [[file]] 180 20
// CHECK-NEXT: OpExtInst %float {{%[0-9]+}} Sqrt [[abs]]
// CHECK: OpLine [[file]] 180 7
// CHECK-NEXT: OpExtInst %uint {{%[0-9]+}} FindUMsb
// CHECK-NEXT: OpExtInst %uint {{%[0-9]+}} FindSMsb
max(firstbithigh(sqrt(abs(v2f.x * v4f.w)) + v4i.x),
// CHECK: OpLine [[file]] 183 7
// CHECK-NEXT: OpExtInst %float {{%[0-9]+}} Cos
Expand Down
6 changes: 6 additions & 0 deletions tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1074,9 +1074,15 @@ TEST_F(FileTest, IntrinsicsFaceForward) {
TEST_F(FileTest, IntrinsicsFirstBitHigh) {
runFileTest("intrinsics.firstbithigh.hlsl");
}
TEST_F(FileTest, IntrinsicsFirstBitHigh64bit) {
runFileTest("intrinsics.firstbithigh.64bit.hlsl", Expect::Failure);
}
TEST_F(FileTest, IntrinsicsFirstBitLow) {
runFileTest("intrinsics.firstbitlow.hlsl");
}
TEST_F(FileTest, IntrinsicsFirstBitLow64bit) {
runFileTest("intrinsics.firstbitlow.64bit.hlsl", Expect::Failure);
}
TEST_F(FileTest, IntrinsicsPrintf) { runFileTest("intrinsics.printf.hlsl"); }
TEST_F(FileTest, IntrinsicsFloor) { runFileTest("intrinsics.floor.hlsl"); }
TEST_F(FileTest, IntrinsicsFmod) { runFileTest("intrinsics.fmod.hlsl"); }
Expand Down