Skip to content

Commit

Permalink
Revert "[SPIR-V] add support for extension KHR_fragment_shading_baryc…
Browse files Browse the repository at this point in the history
…entric (SV_Barycentrics)" (microsoft#5384)

Reverts microsoft#4638
This PR causes multiples issues (microsoft#5326, microsoft#5342, microsoft#5270). Reverting, will
need more testing.

Fixed microsoft#5270
  • Loading branch information
Keenuts authored Jul 4, 2023
1 parent 4c9b6a5 commit 2096d1b
Show file tree
Hide file tree
Showing 19 changed files with 90 additions and 336 deletions.
4 changes: 2 additions & 2 deletions docs/SPIR-V.rst
Original file line number Diff line number Diff line change
Expand Up @@ -297,10 +297,10 @@ Supported extensions
* SPV_EXT_mesh_shader
* SPV_EXT_shader_stencil_support
* SPV_AMD_shader_early_and_late_fragment_tests
* SPV_AMD_shader_explicit_vertex_parameter
* SPV_GOOGLE_hlsl_functionality1
* SPV_GOOGLE_user_type
* SPV_NV_mesh_shader
* SPV_KHR_fragment_shading_barycentric

Vulkan specific attributes
--------------------------
Expand Down Expand Up @@ -1550,7 +1550,7 @@ some system-value (SV) semantic strings will be translated into SPIR-V
+---------------------------+-------------+----------------------------------------+-----------------------+-----------------------------+
| SV_StencilRef | PSOut | ``FragStencilRefEXT`` | N/A | ``StencilExportEXT`` |
+---------------------------+-------------+----------------------------------------+-----------------------+-----------------------------+
| SV_Barycentrics | PSIn | ``BaryCoord*KHR`` | N/A | ``FragmentBarycentricKHR`` |
| SV_Barycentrics | PSIn | ``BaryCoord*AMD`` | N/A | ``Shader`` |
+---------------------------+-------------+----------------------------------------+-----------------------+-----------------------------+
| | GSOut | ``Layer`` | N/A | ``Geometry`` |
| +-------------+----------------------------------------+-----------------------+-----------------------------+
Expand Down
2 changes: 1 addition & 1 deletion tools/clang/include/clang/SPIRV/FeatureManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ enum class Extension {
EXT_shader_viewport_index_layer,
AMD_gpu_shader_half_float,
AMD_shader_early_and_late_fragment_tests,
AMD_shader_explicit_vertex_parameter,
GOOGLE_hlsl_functionality1,
GOOGLE_user_type,
NV_ray_tracing,
Expand All @@ -56,7 +57,6 @@ enum class Extension {
EXT_shader_image_int64,
KHR_physical_storage_buffer,
KHR_vulkan_memory_model,
KHR_fragment_shader_barycentric,
Unknown,
};

Expand Down
3 changes: 0 additions & 3 deletions tools/clang/include/clang/SPIRV/SpirvBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -706,9 +706,6 @@ class SpirvBuilder {
void decoratePerTaskNV(SpirvInstruction *target, uint32_t offset,
SourceLocation);

/// \brief Decorates the given target with PerVertexKHR
void decoratePerVertexKHR(SpirvInstruction *argInst, SourceLocation);

/// \brief Decorates the given target with Coherent
void decorateCoherent(SpirvInstruction *target, SourceLocation);

Expand Down
20 changes: 8 additions & 12 deletions tools/clang/lib/SPIRV/CapabilityVisitor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,6 @@ bool CapabilityVisitor::visit(SpirvDecoration *decor) {
loc);
break;
}
case spv::Decoration::PerVertexKHR: {
addExtension(Extension::KHR_fragment_shader_barycentric, "PerVertexKHR", loc);
addCapability(spv::Capability::FragmentBarycentricKHR);
break;
}
// Capabilities needed for built-ins
case spv::Decoration::BuiltIn: {
AddVulkanMemoryModelForVolatile(decor, loc);
Expand Down Expand Up @@ -365,14 +360,15 @@ bool CapabilityVisitor::visit(SpirvDecoration *decor) {
addCapability(spv::Capability::CullDistance);
break;
}
case spv::BuiltIn::BaryCoordKHR:
case spv::BuiltIn::BaryCoordNoPerspKHR: {
// SV_Barycentrics will have only two builtins
// But it is still allowed to decorate those two builtins with
// interpolation qualifier like centroid or sample.
addExtension(Extension::KHR_fragment_shader_barycentric,
case spv::BuiltIn::BaryCoordNoPerspAMD:
case spv::BuiltIn::BaryCoordNoPerspCentroidAMD:
case spv::BuiltIn::BaryCoordNoPerspSampleAMD:
case spv::BuiltIn::BaryCoordSmoothAMD:
case spv::BuiltIn::BaryCoordSmoothCentroidAMD:
case spv::BuiltIn::BaryCoordSmoothSampleAMD:
case spv::BuiltIn::BaryCoordPullModelAMD: {
addExtension(Extension::AMD_shader_explicit_vertex_parameter,
"SV_Barycentrics", loc);
addCapability(spv::Capability::FragmentBarycentricKHR);
break;
}
case spv::BuiltIn::ShadingRateKHR:
Expand Down
83 changes: 27 additions & 56 deletions tools/clang/lib/SPIRV/DeclResultIdMapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2643,7 +2643,7 @@ bool DeclResultIdMapper::createStageVars(
evalType = astContext.BoolTy;
break;
case hlsl::Semantic::Kind::Barycentrics:
evalType = astContext.getExtVectorType(astContext.FloatTy, 3);
evalType = astContext.getExtVectorType(astContext.FloatTy, 2);
break;
case hlsl::Semantic::Kind::DispatchThreadID:
case hlsl::Semantic::Kind::GroupThreadID:
Expand Down Expand Up @@ -2774,16 +2774,15 @@ bool DeclResultIdMapper::createStageVars(

// Decorate with interpolation modes for pixel shader input variables
// or vertex shader output variables.
if ((spvContext.isPS() && sigPoint->IsInput()) ||
(spvContext.isVS() && sigPoint->IsOutput()))
decorateInterpolationMode(decl, type, varInstr, *semanticToUse);
if (((spvContext.isPS() && sigPoint->IsInput()) ||
(spvContext.isVS() && sigPoint->IsOutput())) &&
// BaryCoord*AMD buitins already encode the interpolation mode.
semanticKind != hlsl::Semantic::Kind::Barycentrics)
decorateInterpolationMode(decl, type, varInstr);

if (asInput) {
if (decl->getAttr<HLSLNoInterpolationAttr>()) {
spvBuilder.decoratePerVertexKHR(varInstr,
varInstr->getSourceLocation());
}
*value = spvBuilder.createLoad(evalType, varInstr, loc);

// Fix ups for corner cases

// Special handling of SV_TessFactor DS patch constant input.
Expand Down Expand Up @@ -2858,8 +2857,8 @@ bool DeclResultIdMapper::createStageVars(
constOne, constZero, thisSemantic.loc);
}
// Special handling of SV_Barycentrics, which is a float3, but the
// The 3 values are NOT guaranteed to add up to floating-point 1.0 exactly.
// Calculate the third element here.
// underlying stage input variable is a float2 (only provides the first
// two components). Calculate the third element.
else if (semanticKind == hlsl::Semantic::Kind::Barycentrics) {
const auto x = spvBuilder.createCompositeExtract(
astContext.FloatTy, *value, {0}, thisSemantic.loc);
Expand Down Expand Up @@ -3350,52 +3349,11 @@ DeclResultIdMapper::invertWIfRequested(SpirvInstruction *position,

void DeclResultIdMapper::decorateInterpolationMode(const NamedDecl *decl,
QualType type,
SpirvVariable *varInstr,
const SemanticInfo semanticInfo)
{
SpirvVariable *varInstr) {
if (varInstr->getStorageClass() != spv::StorageClass::Input &&
varInstr->getStorageClass() != spv::StorageClass::Output) {
return;
}
const bool isBaryCoord = (semanticInfo.getKind() == hlsl::Semantic::Kind::Barycentrics);
uint32_t semanticIndex = semanticInfo.index;

if (isBaryCoord) {
// BaryCentrics inputs cannot have attrib 'nointerpolation'.
if (decl->getAttr<HLSLNoInterpolationAttr>()) {
emitError("SV_BaryCentrics inputs cannot have attribute 'nointerpolation'.",
decl->getLocation());
}
// SV_BaryCentrics could only have two index and apply to different inputs.
// The index should be 0 or 1, each index should be mapped to different
// interpolation type.
if (semanticIndex > 1) {
emitError("The index SV_BaryCentrics semantics could only be 1 or 0.",
decl->getLocation());
}
else if (noPerspBaryCentricsIndex < 2 && perspBaryCentricsIndex < 2) {
emitError("Cannot have more than 2 inputs with SV_BaryCentrics semantics.",
decl->getLocation());
}
else if (decl->getAttr<HLSLNoPerspectiveAttr>()) {
if (noPerspBaryCentricsIndex == 2 && perspBaryCentricsIndex != semanticIndex) {
noPerspBaryCentricsIndex = semanticIndex;
}
else {
emitError("Cannot have more than 1 noperspective inputs with SV_BaryCentrics semantics.",
decl->getLocation());
}
}
else {
if (perspBaryCentricsIndex == 2 && noPerspBaryCentricsIndex != semanticIndex) {
perspBaryCentricsIndex = semanticIndex;
}
else{
emitError("Cannot have more than 1 perspective-correct inputs with SV_BaryCentrics semantics.",
decl->getLocation());
}
}
}

const auto loc = decl->getLocation();
if (isUintOrVecMatOfUintType(type) || isSintOrVecMatOfSintType(type) ||
Expand All @@ -3416,9 +3374,9 @@ void DeclResultIdMapper::decorateInterpolationMode(const NamedDecl *decl,
// Attributes can be used together. So cannot use else if.
if (decl->getAttr<HLSLCentroidAttr>())
spvBuilder.decorateCentroid(varInstr, loc);
if (decl->getAttr<HLSLNoInterpolationAttr>() && !isBaryCoord)
if (decl->getAttr<HLSLNoInterpolationAttr>())
spvBuilder.decorateFlat(varInstr, loc);
if (decl->getAttr<HLSLNoPerspectiveAttr>() && !isBaryCoord)
if (decl->getAttr<HLSLNoPerspectiveAttr>())
spvBuilder.decorateNoPerspective(varInstr, loc);
if (decl->getAttr<HLSLSampleAttr>()) {
spvBuilder.decorateSample(varInstr, loc);
Expand Down Expand Up @@ -3514,6 +3472,7 @@ SpirvVariable *DeclResultIdMapper::createSpirvStageVar(
const auto sigPointKind = sigPoint->GetKind();
const auto type = stageVar->getAstType();
const auto isPrecise = decl->hasAttr<HLSLPreciseAttr>();

spv::StorageClass sc = getStorageClassForSigPoint(sigPoint);
if (sc == spv::StorageClass::Max)
return 0;
Expand Down Expand Up @@ -3787,9 +3746,21 @@ SpirvVariable *DeclResultIdMapper::createSpirvStageVar(
// Selecting the correct builtin according to interpolation mode
auto bi = BuiltIn::Max;
if (decl->hasAttr<HLSLNoPerspectiveAttr>()) {
bi = BuiltIn::BaryCoordNoPerspKHR;
if (decl->hasAttr<HLSLCentroidAttr>()) {
bi = BuiltIn::BaryCoordNoPerspCentroidAMD;
} else if (decl->hasAttr<HLSLSampleAttr>()) {
bi = BuiltIn::BaryCoordNoPerspSampleAMD;
} else {
bi = BuiltIn::BaryCoordNoPerspAMD;
}
} else {
bi = BuiltIn::BaryCoordKHR;
if (decl->hasAttr<HLSLCentroidAttr>()) {
bi = BuiltIn::BaryCoordSmoothCentroidAMD;
} else if (decl->hasAttr<HLSLSampleAttr>()) {
bi = BuiltIn::BaryCoordSmoothSampleAMD;
} else {
bi = BuiltIn::BaryCoordSmoothAMD;
}
}

return spvBuilder.addStageBuiltinVar(type, sc, bi, isPrecise, srcLoc);
Expand Down
5 changes: 1 addition & 4 deletions tools/clang/lib/SPIRV/DeclResultIdMapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,7 @@ class DeclResultIdMapper {
/// Decorates varInstr of the given asType with proper interpolation modes
/// considering the attributes on the given decl.
void decorateInterpolationMode(const NamedDecl *decl, QualType asType,
SpirvVariable *varInstr, const SemanticInfo semanticInfo);
SpirvVariable *varInstr);

/// Returns the proper SPIR-V storage class (Input or Output) for the given
/// SigPoint.
Expand Down Expand Up @@ -893,8 +893,6 @@ class DeclResultIdMapper {
/// an additional SPIR-V optimization pass to flatten such structures.
bool needsFlatteningCompositeResources;

uint32_t perspBaryCentricsIndex, noPerspBaryCentricsIndex;

public:
/// The gl_PerVertex structs for both input and output
GlPerVertex glPerVertex;
Expand Down Expand Up @@ -926,7 +924,6 @@ DeclResultIdMapper::DeclResultIdMapper(ASTContext &context,
spirvOptions(options), astContext(context), spvContext(spirvContext),
diags(context.getDiagnostics()), entryFunction(nullptr),
needsLegalization(false), needsFlatteningCompositeResources(false),
perspBaryCentricsIndex(2), noPerspBaryCentricsIndex(2),
glPerVertex(context, spirvContext, spirvBuilder) {}

bool DeclResultIdMapper::decorateStageIOLocations() {
Expand Down
7 changes: 4 additions & 3 deletions tools/clang/lib/SPIRV/FeatureManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,8 @@ Extension FeatureManager::getExtensionSymbol(llvm::StringRef name) {
Extension::AMD_gpu_shader_half_float)
.Case("SPV_AMD_shader_early_and_late_fragment_tests",
Extension::AMD_shader_early_and_late_fragment_tests)
.Case("SPV_AMD_shader_explicit_vertex_parameter",
Extension::AMD_shader_explicit_vertex_parameter)
.Case("SPV_GOOGLE_hlsl_functionality1",
Extension::GOOGLE_hlsl_functionality1)
.Case("SPV_GOOGLE_user_type", Extension::GOOGLE_user_type)
Expand All @@ -195,7 +197,6 @@ Extension FeatureManager::getExtensionSymbol(llvm::StringRef name) {
.Case("SPV_KHR_physical_storage_buffer",
Extension::KHR_physical_storage_buffer)
.Case("SPV_KHR_vulkan_memory_model", Extension::KHR_vulkan_memory_model)
.Case("SPV_KHR_fragment_shader_barycentric", Extension::KHR_fragment_shader_barycentric)
.Default(Extension::Unknown);
}

Expand Down Expand Up @@ -237,6 +238,8 @@ const char *FeatureManager::getExtensionName(Extension symbol) {
return "SPV_AMD_gpu_shader_half_float";
case Extension::AMD_shader_early_and_late_fragment_tests:
return "SPV_AMD_shader_early_and_late_fragment_tests";
case Extension::AMD_shader_explicit_vertex_parameter:
return "SPV_AMD_shader_explicit_vertex_parameter";
case Extension::GOOGLE_hlsl_functionality1:
return "SPV_GOOGLE_hlsl_functionality1";
case Extension::GOOGLE_user_type:
Expand All @@ -255,8 +258,6 @@ const char *FeatureManager::getExtensionName(Extension symbol) {
return "SPV_KHR_physical_storage_buffer";
case Extension::KHR_vulkan_memory_model:
return "SPV_KHR_vulkan_memory_model";
case Extension::KHR_fragment_shader_barycentric:
return "SPV_KHR_fragment_shader_barycentric";
default:
break;
}
Expand Down
7 changes: 0 additions & 7 deletions tools/clang/lib/SPIRV/SpirvBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1543,13 +1543,6 @@ void SpirvBuilder::decoratePerTaskNV(SpirvInstruction *target, uint32_t offset,
mod->addDecoration(decor);
}

void SpirvBuilder::decoratePerVertexKHR(SpirvInstruction *target,
SourceLocation srcLoc) {
auto *decor = new (context)
SpirvDecoration(srcLoc, target, spv::Decoration::PerVertexKHR);
mod->addDecoration(decor);
}

void SpirvBuilder::decorateCoherent(SpirvInstruction *target,
SourceLocation srcLoc) {
auto *decor =
Expand Down
Loading

0 comments on commit 2096d1b

Please sign in to comment.