Skip to content

Commit

Permalink
Simplify CapabilitySet Diagnostic Printing (shader-slang#4678)
Browse files Browse the repository at this point in the history
Fixes: shader-slang#4675
Fixes: shader-slang#4683
Fixes: shader-slang#4443
Fixes: shader-slang#4585
Fixes: shader-slang#4172

Made the following changes:
1. All capability diagnostic printing logic tries to simplify before printing. This means that we do not print atoms which imply another atom.
2. Do not print the `_` prefix part of atom names since it is misleading users on what they should use to solve a capability issue encountered. (`_Internal` `External` atom changes are not in this PR)
3. Bundle together printing of all sets which contain exactly the same atoms (excluding abstract atoms). This allows printing the following `vertex/fragment/hull/domain/... + glsl` instead of `vertex + glsl | fragment + glsl | hull + glsl | domain + glsl | ....`
4. Rework how entry-point errors are reported to users (example at bottom of PR comment)
5. Rework how atom-provenance data is collected to be leaner and more useful so we can rework the errors. There are 2 notable changes here:
    * We no longer store a list which describes where the first of an `CapabilityAtom` comes from. This heavily simplifies AST logic for the capability system. AST parsing of capabilities is much faster. The trade-off is faster AST parsing and correct AST node data for slower diagnostics if an error is found
    * atom-provenance data now stores a reference to an atom's use-site to provide information on **where** and **what** is wrong with user code versus only sharing **what** and not where.
  • Loading branch information
ArielG-NV authored Jul 23, 2024
1 parent 15f091a commit 509bfd8
Show file tree
Hide file tree
Showing 11 changed files with 387 additions and 74 deletions.
2 changes: 1 addition & 1 deletion source/slang/slang-ast-base.h
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,7 @@ class Decl : public DeclBase
bool isChildOf(Decl* other) const;

// Track the decl reference that caused the requirement of a capability atom.
SLANG_UNREFLECTED Dictionary<CapabilityAtom, DeclReferenceWithLoc> capabilityRequirementProvenance;
SLANG_UNREFLECTED List<DeclReferenceWithLoc> capabilityRequirementProvenance;
private:
SLANG_UNREFLECTED DeclRefBase* m_defaultDeclRef = nullptr;
SLANG_UNREFLECTED Index m_defaultDeclRefEpoch = -1;
Expand Down
183 changes: 158 additions & 25 deletions source/slang/slang-capability.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,57 @@ bool isCapabilityDerivedFrom(CapabilityAtom atom, CapabilityAtom base)
return false;
}

//CapabilityAtomSet

CapabilityAtomSet CapabilityAtomSet::newSetWithoutImpliedAtoms() const
{
// plan is to add all atoms which is impled (=>) another atom.
// Implying an atom appears in the form of atom1=>atom2 or atom2=>atom1.
Dictionary<CapabilityAtom, bool> candidateForSimplifiedList;
CapabilityAtomSet simplifiedSet;
for (auto atom1UInt : *this)
{
CapabilityAtom atom1 = (CapabilityAtom)atom1UInt;
if (!candidateForSimplifiedList.addIfNotExists(atom1, true)
&& candidateForSimplifiedList[atom1] == false)
continue;

for (auto atom2UInt : *this)
{
if (atom1UInt == atom2UInt)
continue;

CapabilityAtom atom2 = (CapabilityAtom)atom2UInt;
if (!candidateForSimplifiedList.addIfNotExists(atom2, true)
&& candidateForSimplifiedList[atom2] == false)
continue;

auto atomInfo1 = _getInfo(atom1).canonicalRepresentation;
auto atomInfo2 = _getInfo(atom2).canonicalRepresentation;
for (auto atomSet1 : atomInfo1)
{
for (auto atomSet2 : atomInfo2)
{
if (atomSet1->contains(*atomSet2))
{
candidateForSimplifiedList[atom2] = false;
continue;
}
else if (atomSet2->contains(*atomSet1))
{
candidateForSimplifiedList[atom1] = false;
continue;
}
}
}
}
}
for (auto i : candidateForSimplifiedList)
if(i.second)
simplifiedSet.add((UInt)i.first);
return simplifiedSet;
}

//// CapabiltySet

CapabilityAtom getTargetAtomInSet(const CapabilityAtomSet& atomSet)
Expand Down Expand Up @@ -897,31 +948,118 @@ void CapabilitySet::addSpirvVersionFromOtherAsGlslSpirvVersion(CapabilitySet& ot
}
}

void printDiagnosticArg(StringBuilder& sb, const CapabilitySet& capSet)
UnownedStringSlice capabilityNameToStringWithoutPrefix(CapabilityName capabilityName)
{
auto name = capabilityNameToString(capabilityName);
if (name.startsWith("_"))
return name.tail(1);
return name;
}

void printDiagnosticArg(StringBuilder& sb, const CapabilityAtomSet atomSet)
{
bool isFirst = true;
for (auto atom : atomSet.newSetWithoutImpliedAtoms())
{
CapabilityName formattedAtom = (CapabilityName)atom;
if (!isFirst)
sb << " + ";
sb << capabilityNameToStringWithoutPrefix(formattedAtom);
isFirst = false;
}
}

// Collection of stages which have same atom sets to compress reprisentation of atom and stage per target
struct CompressedCapabilitySet
{
bool isFirstSet = true;
for (auto& set : capSet.getAtomSets())
/// Collection of stages which have same atom sets to compress reprisentation of atom and stage: {vertex/fragment, ... }
struct StageAndAtomSet
{
CapabilityAtomSet stages;
CapabilityAtomSet atomsWithoutStage;
};

auto begin()
{
if (!isFirstSet)
return atomSetsOfTargets.begin();
}

/// Compress 1 capabilitySet into a reprisentation which merges stages that share all of their atoms for printing.
Dictionary<CapabilityAtom, List<StageAndAtomSet>> atomSetsOfTargets;
CompressedCapabilitySet(const CapabilitySet& capabilitySet)
{
for (auto& atomSet : capabilitySet.getAtomSets())
{
sb<< " | ";
auto target = getTargetAtomInSet(atomSet);

auto stageInSetAtom = getStageAtomInSet(atomSet);
CapabilityAtomSet stageInSet;
stageInSet.add((UInt)stageInSetAtom);

CapabilityAtomSet atomsWithoutStage;
CapabilityAtomSet::calcSubtract(atomsWithoutStage, atomSet, stageInSet);
if (!atomSetsOfTargets.containsKey(target))
{
atomSetsOfTargets[target].add({ stageInSet, atomsWithoutStage });
continue;
}

// try to find an equivlent atom set by iterating all of the same `atomSetsOfTarget[target]` and merge these 2 together.
auto& atomSetsOfTarget = atomSetsOfTargets[target];
for (auto& i : atomSetsOfTarget)
{
if (i.atomsWithoutStage.contains(atomsWithoutStage) && atomsWithoutStage.contains(i.atomsWithoutStage))
{
i.stages.add((UInt)stageInSetAtom);
}
}
}
bool isFirst = true;
for (auto atom : set)
for (auto& targetSets : atomSetsOfTargets)
for (auto& targetSet : targetSets.second)
targetSet.atomsWithoutStage = targetSet.atomsWithoutStage.newSetWithoutImpliedAtoms();
}
};

void printDiagnosticArg(StringBuilder& sb, const CompressedCapabilitySet& capabilitySet)
{
////Secondly we will print our new list of atomSet's.
sb << "{";
bool firstSet = true;
for (auto targetSets : capabilitySet.atomSetsOfTargets)
{
if(!firstSet)
sb << " || ";
for (auto targetSet : targetSets.second)
{
CapabilityName formattedAtom = (CapabilityName)atom;
if (!isFirst)
bool firstStage = true;
for (auto stageAtom : targetSet.stages)
{
if (!firstStage)
sb << "/";
printDiagnosticArg(sb, (CapabilityName)stageAtom);
firstStage = false;
}
for (auto atom : targetSet.atomsWithoutStage)
{
sb << " + ";
printDiagnosticArg(sb, (CapabilityName)atom);
}
auto name = capabilityNameToString((CapabilityName)formattedAtom);
if (name.startsWith("_"))
name = name.tail(1);
sb << name;
isFirst = false;
}
isFirstSet = false;
firstSet = false;
}
sb << "}";
}

void printDiagnosticArg(StringBuilder& sb, const CapabilitySet& capabilitySet)
{
// Firstly we will compress the printing of capabilities such that any atomSet
// with different abstract atoms but equal non-abstract atoms will be bundled together.
if (capabilitySet.isInvalid() || capabilitySet.isEmpty())
{
sb << "{}";
return;
}
printDiagnosticArg(sb, CompressedCapabilitySet(capabilitySet));
}

void printDiagnosticArg(StringBuilder& sb, CapabilityAtom atom)
Expand All @@ -931,20 +1069,15 @@ void printDiagnosticArg(StringBuilder& sb, CapabilityAtom atom)

void printDiagnosticArg(StringBuilder& sb, CapabilityName name)
{
sb << _getInfo(name).name;
sb << capabilityNameToStringWithoutPrefix(name);
}

void printDiagnosticArg(StringBuilder& sb, List<CapabilityAtom>& list)
{
sb << "{";
auto count = list.getCount();
for(Index i = 0; i < count; i++)
{
printDiagnosticArg(sb, list[i]);
if (i + 1 != count)
sb << ", ";
}
sb << "}";
CapabilityAtomSet set;
for (auto i : list)
set.add((UInt)i);
printDiagnosticArg(sb, set.newSetWithoutImpliedAtoms());
}


Expand Down
18 changes: 18 additions & 0 deletions source/slang/slang-capability.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ namespace Slang
struct CapabilityAtomSet : UIntSet
{
using UIntSet::UIntSet;

CapabilityAtomSet newSetWithoutImpliedAtoms() const;
};

struct CapabilityTargetSet;
Expand Down Expand Up @@ -300,6 +302,22 @@ struct CapabilitySet
/// Add spirv version capabilities from 'spirv CapabilityTargetSet' as glsl_spirv version capability in 'glsl CapabilityTargetSet'
void addSpirvVersionFromOtherAsGlslSpirvVersion(CapabilitySet& other);

/// Gets the first valid compile-target found in the CapabilitySet
CapabilityAtom getCompileTarget()
{
if(isEmpty() || isInvalid())
return CapabilityAtom::Invalid;
return (*m_targetSets.begin()).first;
}

/// Gets the first valid stage found in the CapabilitySet
CapabilityAtom getTargetStage()
{
if(isEmpty() || isInvalid())
return CapabilityAtom::Invalid;
return (*(*m_targetSets.begin()).second.shaderStageSets.begin()).first;
}

private:
/// underlying data of CapabilitySet.
CapabilityTargetSets m_targetSets{};
Expand Down
Loading

0 comments on commit 509bfd8

Please sign in to comment.