Skip to content
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.

Sign up for GitHub

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

Report error for unsupported types of SV semantics #3043

Merged
merged 24 commits into from
Feb 26, 2021
Merged

Report error for unsupported types of SV semantics #3043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
5ae34bf
Report error on unsupported types for CS input semantics
Jul 16, 2020
772847e
Undo changes in SemaHLSL
Jul 16, 2020
8e245ac
Offline feedback from Tex
Jul 17, 2020
8575cf8
Update test
Jul 17, 2020
8771fb4
Fix travis build error
Jul 17, 2020
79bb9bb
Use DXASSERT
Jul 17, 2020
f6f9ebd
Handle matrix
Jul 17, 2020
1f628e1
Fix indentation
Jul 17, 2020
35266e9
Revert previous change
Dec 24, 2020
4006a27
Generate error for disallowed types for CS input semantics
Dec 24, 2020
9a80b70
Merge branch 'master' of https://github.com/microsoft/DirectXShaderCo…
Dec 25, 2020
953457e
Revert "Generate error for disallowed types for CS input semantics"
Feb 9, 2021
1238ade
Merge branch 'master' of https://github.com/microsoft/DirectXShaderCo…
Feb 9, 2021
34d0e50
Extend type checking for all semantics
Feb 9, 2021
fda69bb
Fix travis build error
Feb 9, 2021
581fe30
Use dxilutil::IsIntegerOrFloatingPointType
Feb 11, 2021
61f2575
Merge branch 'master' of https://github.com/microsoft/DirectXShaderCo…
vcsharma Feb 17, 2021
5bac07c
Merge branch 'master' of https://github.com/microsoft/DirectXShaderCo…
vcsharma Feb 22, 2021
2ec8f4f
Merge branch 'master' of https://github.com/microsoft/DirectXShaderCo…
vcsharma Feb 23, 2021
51bfb18
Accomodate change in signature for dxilutil::EmitErrorOnFunction
vcsharma Feb 23, 2021
1c4767f
Review comments
vcsharma Feb 23, 2021
29c2101
Relax the SV type checking criteria based on internal testing
vcsharma Feb 24, 2021
533a4dd
Merge branch 'master' of https://github.com/microsoft/DirectXShaderCo…
vcsharma Feb 25, 2021
53d077f
Review comments
vcsharma Feb 25, 2021
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
38 changes: 36 additions & 2 deletions include/dxc/DXIL/DxilSemantic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#pragma once

#include "llvm/ADT/StringRef.h"
#include "llvm/IR/Type.h"

#include "DxilConstants.h"
#include "DxilShaderModel.h"
Expand All @@ -23,6 +24,33 @@ class Semantic {
public:
using Kind = DXIL::SemanticKind;

enum class CompTy {
BoolTy = 1 << 0,
HalfTy = 1 << 1,
Int16Ty = 1 << 2,
FloatTy = 1 << 3,
Int32Ty = 1 << 4,
DoubleTy = 1 << 5,
Int64Ty = 1 << 6,
HalfOrFloatTy = HalfTy | FloatTy,
BoolOrInt32Ty = BoolTy | Int32Ty,
BoolOrInt16Or32Ty = BoolTy | Int16Ty | Int32Ty,
FloatOrInt32Ty = FloatTy | Int32Ty,
Int16Or32Ty = Int16Ty | Int32Ty,
AnyIntTy = BoolTy | Int16Ty | Int32Ty | Int64Ty,
AnyFloatTy = HalfTy | FloatTy | DoubleTy,
AnyTy = AnyIntTy | AnyFloatTy,
};

enum class SizeClass {
Unknown,
Scalar,
Vec2,
Vec3,
Vec4,
Other
};

static const int kUndefinedRow = -1;
static const int kUndefinedCol = -1;

Expand All @@ -41,13 +69,19 @@ class Semantic {
const char *GetName() const;
bool IsArbitrary() const;
bool IsInvalid() const;
bool IsSupportedType(llvm::Type *semTy) const;
CompTy GetCompType(llvm::Type* ty) const;
SizeClass GetCompCount(llvm::Type* ty) const;

private:
Kind m_Kind; // Semantic kind.
const char *m_pszName; // Canonical name (for system semantics).
const char *m_pszName; // Canonical name (for system semantics).
CompTy m_allowedTys; // Types allowed for the semantic
SizeClass m_minCompCount; // Minimum component count that is allowed for a semantic
SizeClass m_maxCompCount; // Maximum component count that is allowed for a semantic

Semantic() = delete;
Semantic(Kind Kind, const char *pszName);
Semantic(Kind Kind, const char *pszName, CompTy allowedTys, SizeClass minCompCount, SizeClass maxCompCount);

// Table of all semantic properties.
static const unsigned kNumSemanticRecords = (unsigned)Kind::Invalid + 1;
Expand Down
191 changes: 158 additions & 33 deletions lib/DXIL/DxilSemantic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "dxc/DXIL/DxilSemantic.h"
#include "dxc/DXIL/DxilSignature.h"
#include "dxc/DXIL/DxilShaderModel.h"
#include "dxc/DXIL/DxilUtil.h"
#include "dxc/Support/Global.h"

#include <string>
Expand All @@ -24,9 +25,15 @@ namespace hlsl {
// Semantic class methods.
//
Semantic::Semantic(Kind Kind,
const char *pszName)
const char *pszName,
CompTy allowedTys,
SizeClass minCompCount,
SizeClass maxCompCount)
: m_Kind(Kind)
, m_pszName(pszName)
, m_allowedTys(allowedTys)
, m_minCompCount(minCompCount)
, m_maxCompCount(maxCompCount)
{
}

Expand Down Expand Up @@ -113,41 +120,159 @@ bool Semantic::IsInvalid() const {
return m_Kind == Kind::Invalid;
}

Semantic::SizeClass Semantic::GetCompCount(llvm::Type* ty) const {

if (!ty->isVectorTy() && !dxilutil::IsIntegerOrFloatingPointType(ty))
return SizeClass::Unknown;

if (ty->isVectorTy()) {
if (ty->getVectorNumElements() == 1) {
return SizeClass::Scalar;
}
else if (ty->getVectorNumElements() == 2) {
return SizeClass::Vec2;
}
else if (ty->getVectorNumElements() == 3) {
return SizeClass::Vec3;
}
else if (ty->getVectorNumElements() == 4) {
return SizeClass::Vec4;
}
else {
DXASSERT(false, "Unexpected number of vector elements.");
return SizeClass::Unknown;
}
}

return SizeClass::Scalar;
}

Semantic::CompTy Semantic::GetCompType(llvm::Type* ty) const {

if (!ty->isVectorTy() && !dxilutil::IsIntegerOrFloatingPointType(ty))
return CompTy::AnyTy;

if (ty->isVectorTy())
ty = ty->getScalarType();

// must be an integer or a floating point type here
DXASSERT_NOMSG(dxilutil::IsIntegerOrFloatingPointType(ty));
if (ty->getScalarType()->isIntegerTy()) {
if (ty->getScalarSizeInBits() == 1) {
return CompTy::BoolTy;
} else if (ty->getScalarSizeInBits() == 16) {
return CompTy::Int16Ty;
} else if (ty->getScalarSizeInBits() == 32) {
return CompTy::Int32Ty;
} else {
return CompTy::Int64Ty;
}
} else {
if (ty->isHalfTy()) {
return CompTy::HalfTy;
} else if (ty->isFloatTy()) {
return CompTy::FloatTy;
} else {
DXASSERT_NOMSG(ty->isDoubleTy());
return CompTy::DoubleTy;
}
}
}

static bool IsScalarOrVectorTy(llvm::Type* ty) {
if (dxilutil::IsIntegerOrFloatingPointType(ty))
return true;
if (ty->isVectorTy() &&
dxilutil::IsIntegerOrFloatingPointType(ty->getVectorElementType()))
return true;
return false;
}

bool Semantic::IsSupportedType(llvm::Type* semTy) const {

if (m_Kind == Kind::Invalid)
return false;

// Skip type checking for Arbitrary kind
if (m_Kind == Kind::Arbitrary)
return true;

if (!IsScalarOrVectorTy(semTy)) {
// We only allow scalar or vector types as a valid semantic type except in some cases
// such as Clip/Cull or Tessfactor.
if (m_minCompCount == SizeClass::Other) {
if (semTy->isArrayTy()) {
semTy = semTy->getArrayElementType();
// TessFactor or InsideTessFactor must either be float[2] or float
if ((m_Kind == Kind::TessFactor ||
m_Kind == Kind::InsideTessFactor) &&
!dxilutil::IsIntegerOrFloatingPointType(semTy)) {
return false;
}
// Clip/Cull can be array of scalar or vector
if ((m_Kind == Kind::ClipDistance ||
m_Kind == Kind::CullDistance) &&
!IsScalarOrVectorTy(semTy)) {
return false;
}
}
else {
// Do not support other types such as matrix.
return false;
}
}
else {
return false;
}
}

if (((unsigned)m_allowedTys & (unsigned)GetCompType(semTy)) == 0)
return false;

// Skip type-shape validation for semantics marked as Other
if (m_minCompCount == SizeClass::Other)
return true;

SizeClass compSzClass = GetCompCount(semTy);
return compSzClass >= m_minCompCount &&
compSzClass <= m_maxCompCount;
}

typedef Semantic SP;
const Semantic Semantic::ms_SemanticTable[kNumSemanticRecords] = {
// Kind Name
SP(Kind::Arbitrary, nullptr),
SP(Kind::VertexID, "SV_VertexID"),
SP(Kind::InstanceID, "SV_InstanceID"),
SP(Kind::Position, "SV_Position"),
SP(Kind::RenderTargetArrayIndex,"SV_RenderTargetArrayIndex"),
SP(Kind::ViewPortArrayIndex, "SV_ViewportArrayIndex"),
SP(Kind::ClipDistance, "SV_ClipDistance"),
SP(Kind::CullDistance, "SV_CullDistance"),
SP(Kind::OutputControlPointID, "SV_OutputControlPointID"),
SP(Kind::DomainLocation, "SV_DomainLocation"),
SP(Kind::PrimitiveID, "SV_PrimitiveID"),
SP(Kind::GSInstanceID, "SV_GSInstanceID"),
SP(Kind::SampleIndex, "SV_SampleIndex"),
SP(Kind::IsFrontFace, "SV_IsFrontFace"),
SP(Kind::Coverage, "SV_Coverage"),
SP(Kind::InnerCoverage, "SV_InnerCoverage"),
SP(Kind::Target, "SV_Target"),
SP(Kind::Depth, "SV_Depth"),
SP(Kind::DepthLessEqual, "SV_DepthLessEqual"),
SP(Kind::DepthGreaterEqual, "SV_DepthGreaterEqual"),
SP(Kind::StencilRef, "SV_StencilRef"),
SP(Kind::DispatchThreadID, "SV_DispatchThreadID"),
SP(Kind::GroupID, "SV_GroupID"),
SP(Kind::GroupIndex, "SV_GroupIndex"),
SP(Kind::GroupThreadID, "SV_GroupThreadID"),
SP(Kind::TessFactor, "SV_TessFactor"),
SP(Kind::InsideTessFactor, "SV_InsideTessFactor"),
SP(Kind::ViewID, "SV_ViewID"),
SP(Kind::Barycentrics, "SV_Barycentrics"),
SP(Kind::ShadingRate, "SV_ShadingRate"),
SP(Kind::CullPrimitive, "SV_CullPrimitive"),
SP(Kind::Invalid, nullptr),
SP(Kind::Arbitrary, nullptr, CompTy::AnyTy, SizeClass::Other, SizeClass::Other),
SP(Kind::VertexID, "SV_VertexID", CompTy::Int16Or32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::InstanceID, "SV_InstanceID", CompTy::Int16Or32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::Position, "SV_Position", CompTy::HalfOrFloatTy, SizeClass::Vec4, SizeClass::Vec4),
SP(Kind::RenderTargetArrayIndex,"SV_RenderTargetArrayIndex", CompTy::Int16Or32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::ViewPortArrayIndex, "SV_ViewportArrayIndex", CompTy::Int16Or32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::ClipDistance, "SV_ClipDistance", CompTy::HalfOrFloatTy, SizeClass::Other, SizeClass::Other),
SP(Kind::CullDistance, "SV_CullDistance", CompTy::HalfOrFloatTy, SizeClass::Other, SizeClass::Other),
SP(Kind::OutputControlPointID, "SV_OutputControlPointID", CompTy::Int32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::DomainLocation, "SV_DomainLocation", CompTy::FloatTy, SizeClass::Scalar, SizeClass::Vec3),
SP(Kind::PrimitiveID, "SV_PrimitiveID", CompTy::Int32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::GSInstanceID, "SV_GSInstanceID", CompTy::Int32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::SampleIndex, "SV_SampleIndex", CompTy::Int32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::IsFrontFace, "SV_IsFrontFace", CompTy::BoolOrInt32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::Coverage, "SV_Coverage", CompTy::Int32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::InnerCoverage, "SV_InnerCoverage", CompTy::Int32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::Target, "SV_Target", CompTy::AnyTy, SizeClass::Scalar, SizeClass::Vec4),
SP(Kind::Depth, "SV_Depth", CompTy::HalfOrFloatTy, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::DepthLessEqual, "SV_DepthLessEqual", CompTy::HalfOrFloatTy, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::DepthGreaterEqual, "SV_DepthGreaterEqual", CompTy::HalfOrFloatTy, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::StencilRef, "SV_StencilRef", CompTy::Int16Or32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::DispatchThreadID, "SV_DispatchThreadID", CompTy::Int16Or32Ty, SizeClass::Scalar, SizeClass::Vec3),
SP(Kind::GroupID, "SV_GroupID", CompTy::Int16Or32Ty, SizeClass::Scalar, SizeClass::Vec3),
SP(Kind::GroupIndex, "SV_GroupIndex", CompTy::Int16Or32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::GroupThreadID, "SV_GroupThreadID", CompTy::Int16Or32Ty, SizeClass::Scalar, SizeClass::Vec3),
SP(Kind::TessFactor, "SV_TessFactor", CompTy::HalfOrFloatTy, SizeClass::Other, SizeClass::Other),
SP(Kind::InsideTessFactor, "SV_InsideTessFactor", CompTy::HalfOrFloatTy, SizeClass::Other, SizeClass::Other),
SP(Kind::ViewID, "SV_ViewID", CompTy::Int32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::Barycentrics, "SV_Barycentrics", CompTy::HalfOrFloatTy, SizeClass::Vec3, SizeClass::Vec3),
SP(Kind::ShadingRate, "SV_ShadingRate", CompTy::Int16Or32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::CullPrimitive, "SV_CullPrimitive", CompTy::BoolOrInt16Or32Ty, SizeClass::Scalar, SizeClass::Scalar),
SP(Kind::Invalid, nullptr, CompTy::AnyTy, SizeClass::Other, SizeClass::Other),
};

} // namespace hlsl
50 changes: 50 additions & 0 deletions lib/HLSL/HLSignatureLower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1210,6 +1210,50 @@ void HLSignatureLower::GenerateDxilInputsOutputs(DXIL::SignatureKind SK) {
}
}

bool HLSignatureLower::ValidateSemanticType(llvm::Function* F) {
bool result = true;
DxilFunctionAnnotation* funcAnnotation = HLM.GetFunctionAnnotation(F);
if (!funcAnnotation) {
return result;
}
for (Argument& arg : F->args()) {
DxilParameterAnnotation &paramAnnotation =
funcAnnotation->GetParameterAnnotation(arg.getArgNo());
llvm::StringRef semanticStr = paramAnnotation.GetSemanticString();
if (semanticStr.empty()) {
continue;
}
unsigned index;
StringRef baseSemanticStr;
Semantic::DecomposeNameAndIndex(semanticStr, &baseSemanticStr, &index);
const Semantic* semantic = Semantic::GetByName(baseSemanticStr);
Type* argTy = arg.getType();

if (argTy->isPointerTy())
argTy = cast<PointerType>(argTy)->getPointerElementType();

if (argTy->isArrayTy()) {
// Array type for arguments with specific qualifiers are expected.
// In this case, we do validation on array's element type.
DxilParamInputQual inputQual = paramAnnotation.GetParamInputQual();
if (inputQual == DxilParamInputQual::InputPatch ||
inputQual == DxilParamInputQual::InputPrimitive ||
inputQual == DxilParamInputQual::OutIndices ||
inputQual == DxilParamInputQual::OutPrimitives ||
inputQual == DxilParamInputQual::OutputPatch ||
inputQual == DxilParamInputQual::OutVertices) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wish there was a better place to associate the input qualifier property with the expected array parameter shape (though this is a bit of an internal HL IR detail). Even if it was in its own function somewhere.

argTy = cast<ArrayType>(argTy)->getArrayElementType();
}
}

if (!semantic->IsSupportedType(argTy)) {
dxilutil::EmitErrorOnFunction(F->getContext(), F, "invalid type used for \'"+ semanticStr.str() + "\' semantic.");
result = false;
}
}
return result;
}

void HLSignatureLower::GenerateDxilCSInputs() {
OP *hlslOP = HLM.GetOP();

Expand Down Expand Up @@ -1721,6 +1765,12 @@ void HLSignatureLower::GenerateGetMeshPayloadOperation() {
}
// Lower signatures.
void HLSignatureLower::Run() {

// Generate error and exit if semantic type
// is not one of the allowed types
if (!ValidateSemanticType(Entry))
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This provides no indication of failure to the subsequent passes, I worry this will bail out of the rest of this pass and potentially cause weird followup errors on subsequent passes that expect these operations to have been performed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, and I have a feeling that continuing with this pass will result in fewer weird cascading errors from this than returning here. Currently, it seems the only way to abort the pipeline is with a report_fatal_error, which currently raises a structured exception - which is way too harsh for something like this, and we should likely change report_fatal_error to throw a C++ exception, since we still have a chance of emitting a useful error message then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling that continuing with this pass will result in fewer weird cascading errors from this than returning here

Actually bailing out early avoids crashes at least in some cases. Like for CS SV semantics, if we pass an invalid type like float and let it continue with the lowering then it crashes in HLSignatureLower::GenerateDxilCSInputs() in the below line.

    // If the argument is of non-i32 type, convert here
    if (newArg->getType() != NumTy)
      newArg = Builder.CreateZExtOrTrunc(newArg, NumTy);

Based on the comments here, I agree we need a mechanism to stop further compilation when we generate errors. I suspect we would need that mechanism not just in this change, but probably at other places too where we generate errors. Therefore I created an issue #3496 to track this as a future enhancement that we would like to implement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's consistent with my later investigations. I approve of solving the larger problem right later.


DxilFunctionProps &props = HLM.GetDxilFunctionProps(Entry);
if (props.IsGraphics()) {
if (props.IsMS()) {
Expand Down
3 changes: 3 additions & 0 deletions lib/HLSL/HLSignatureLower.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
#include <unordered_set>
#include <unordered_map>
#include "dxc/DXIL/DxilConstants.h"
#include "llvm/IR/Function.h"
#include "llvm/IR/Argument.h"

namespace llvm {
class Value;
Expand Down Expand Up @@ -53,6 +55,7 @@ class HLSignatureLower {
void GenerateDxilPrimOutputs();
void GenerateDxilInputsOutputs(DXIL::SignatureKind SK);
void GenerateDxilCSInputs();
bool ValidateSemanticType(llvm::Function* F);
void GenerateDxilPatchConstantLdSt();
void GenerateDxilPatchConstantFunctionInputs();
void GenerateClipPlanesForVS(llvm::Value *outPosition);
Expand Down
Loading