Skip to content

Commit

Permalink
SPV: RelaxedPrecision: Generalize fix #2293 to cover more operations.
Browse files Browse the repository at this point in the history 
This simplifies and enforces use of precision in many more places,
to help avoid accidental loss of RelaxedPrecision through intermediate
operations. Known fixes are:
- ?:
- function return values with mis-matched precision
- precision of function return values when a copy was needed to fix types
  • Loading branch information
@johnkslang
johnkslang committed Jun 30, 2020
1 parent 27e915e commit 435dd80
Show file tree
Hide file tree
Showing 10 changed files with 835 additions and 670 deletions.
44 changes: 21 additions & 23 deletions SPIRV/GlslangToSpv.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2981,7 +2981,8 @@ bool TGlslangToSpvTraverser::visitAggregate(glslang::TVisit visit, glslang::TInt
// receive the result, and must later swizzle that into the original
// l-value.
complexLvalues.push_back(builder.getAccessChain());
temporaryLvalues.push_back(builder.createVariable(spv::StorageClassFunction,
temporaryLvalues.push_back(builder.createVariable(
spv::NoPrecision, spv::StorageClassFunction,
builder.accessChainGetInferredType(), "swizzleTemp"));
operands.push_back(temporaryLvalues.back());
} else {
Expand Down Expand Up @@ -3084,7 +3085,7 @@ bool TGlslangToSpvTraverser::visitAggregate(glslang::TVisit visit, glslang::TInt

for (unsigned int i = 0; i < temporaryLvalues.size(); ++i) {
builder.setAccessChain(complexLvalues[i]);
builder.accessChainStore(builder.createLoad(temporaryLvalues[i]));
builder.accessChainStore(builder.createLoad(temporaryLvalues[i], spv::NoPrecision));
}
}

Expand Down Expand Up @@ -3201,7 +3202,8 @@ bool TGlslangToSpvTraverser::visitSelection(glslang::TVisit /* visit */, glslang
} else {
// We need control flow to select the result.
// TODO: Once SPIR-V OpSelect allows arbitrary types, eliminate this path.
result = builder.createVariable(spv::StorageClassFunction, convertGlslangToSpvType(node->getType()));
result = builder.createVariable(TranslatePrecisionDecoration(node->getType()),
spv::StorageClassFunction, convertGlslangToSpvType(node->getType()));

// Selection control:
const spv::SelectionControlMask control = TranslateSelectionControl(*node);
Expand All @@ -3226,8 +3228,10 @@ bool TGlslangToSpvTraverser::visitSelection(glslang::TVisit /* visit */, glslang
// Execute the one side needed, as per the condition
const auto executeOneSide = [&]() {
// Always emit control flow.
if (node->getBasicType() != glslang::EbtVoid)
result = builder.createVariable(spv::StorageClassFunction, convertGlslangToSpvType(node->getType()));
if (node->getBasicType() != glslang::EbtVoid) {
result = builder.createVariable(TranslatePrecisionDecoration(node->getType()), spv::StorageClassFunction,
convertGlslangToSpvType(node->getType()));
}

// Selection control:
const spv::SelectionControlMask control = TranslateSelectionControl(*node);
Expand Down Expand Up @@ -3424,15 +3428,17 @@ bool TGlslangToSpvTraverser::visitBranch(glslang::TVisit /* visit */, glslang::T
builder.createLoopContinue();
break;
case glslang::EOpReturn:
if (node->getExpression()) {
if (node->getExpression() != nullptr) {
const glslang::TType& glslangReturnType = node->getExpression()->getType();
spv::Id returnId = accessChainLoad(glslangReturnType);
if (builder.getTypeId(returnId) != currentFunction->getReturnType()) {
if (builder.getTypeId(returnId) != currentFunction->getReturnType() ||
TranslatePrecisionDecoration(glslangReturnType) != currentFunction->getReturnPrecision()) {
builder.clearAccessChain();
spv::Id copyId = builder.createVariable(spv::StorageClassFunction, currentFunction->getReturnType());
spv::Id copyId = builder.createVariable(currentFunction->getReturnPrecision(),
spv::StorageClassFunction, currentFunction->getReturnType());
builder.setAccessChainLValue(copyId);
multiTypeStore(glslangReturnType, returnId);
returnId = builder.createLoad(copyId);
returnId = builder.createLoad(copyId, currentFunction->getReturnPrecision());
}
builder.makeReturn(false, returnId);
} else
Expand Down Expand Up @@ -3539,7 +3545,7 @@ spv::Id TGlslangToSpvTraverser::createSpvVariable(const glslang::TIntermSymbol*
false /* specConst */);
}

return builder.createVariable(storageClass, spvType, name, initializer);
return builder.createVariable(spv::NoPrecision, storageClass, spvType, name, initializer);
}

// Return type Id of the sampled type.
Expand Down Expand Up @@ -5334,10 +5340,8 @@ spv::Id TGlslangToSpvTraverser::handleUserFunctionCall(const glslang::TIntermAgg
++lValueCount;
} else if (writableParam(qualifiers[a])) {
// need space to hold the copy
arg = builder.createVariable(spv::StorageClassFunction,
arg = builder.createVariable(function->getParamPrecision(a), spv::StorageClassFunction,
builder.getContainedTypeId(function->getParamType(a)), "param");
if (function->isReducedPrecisionParam(a))
builder.setPrecision(arg, spv::DecorationRelaxedPrecision);
if (qualifiers[a] == glslang::EvqIn || qualifiers[a] == glslang::EvqInOut) {
// need to copy the input into output space
builder.setAccessChain(lValues[lValueCount]);
Expand All @@ -5348,21 +5352,15 @@ spv::Id TGlslangToSpvTraverser::handleUserFunctionCall(const glslang::TIntermAgg
}
++lValueCount;
} else {
const bool argIsRelaxedPrecision = TranslatePrecisionDecoration(*argTypes[a]) ==
spv::DecorationRelaxedPrecision;
// process r-value, which involves a copy for a type mismatch
if (function->getParamType(a) != convertGlslangToSpvType(*argTypes[a]) ||
argIsRelaxedPrecision != function->isReducedPrecisionParam(a))
TranslatePrecisionDecoration(*argTypes[a]) != function->getParamPrecision(a))
{
spv::Id argCopy = builder.createVariable(spv::StorageClassFunction, function->getParamType(a), "arg");
if (function->isReducedPrecisionParam(a))
builder.setPrecision(argCopy, spv::DecorationRelaxedPrecision);
spv::Id argCopy = builder.createVariable(function->getParamPrecision(a), spv::StorageClassFunction, function->getParamType(a), "arg");
builder.clearAccessChain();
builder.setAccessChainLValue(argCopy);
multiTypeStore(*argTypes[a], rValues[rValueCount]);
arg = builder.createLoad(argCopy);
if (function->isReducedPrecisionParam(a))
builder.setPrecision(arg, spv::DecorationRelaxedPrecision);
arg = builder.createLoad(argCopy, function->getParamPrecision(a));
} else
arg = rValues[rValueCount];
++rValueCount;
Expand All @@ -5381,7 +5379,7 @@ spv::Id TGlslangToSpvTraverser::handleUserFunctionCall(const glslang::TIntermAgg
++lValueCount;
else if (writableParam(qualifiers[a])) {
if (qualifiers[a] == glslang::EvqOut || qualifiers[a] == glslang::EvqInOut) {
spv::Id copy = builder.createLoad(spvArgs[a]);
spv::Id copy = builder.createLoad(spvArgs[a], spv::NoPrecision);
builder.setAccessChain(lValues[lValueCount]);
multiTypeStore(*argTypes[a], copy);
}
Expand Down
25 changes: 13 additions & 12 deletions SPIRV/SpvBuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1297,11 +1297,11 @@ Function* Builder::makeFunctionEntry(Decoration precision, Id returnType, const

// Set up the precisions
setPrecision(function->getId(), precision);
function->setReturnPrecision(precision);
for (unsigned p = 0; p < (unsigned)decorations.size(); ++p) {
for (int d = 0; d < (int)decorations[p].size(); ++d) {
addDecoration(firstParamId + p, decorations[p][d]);
if (decorations[p][d] == DecorationRelaxedPrecision)
function->addReducedPrecisionParam(p);
function->addParamPrecision(p, decorations[p][d]);
}
}

Expand Down Expand Up @@ -1359,7 +1359,7 @@ void Builder::makeDiscard()
}

// Comments in header
Id Builder::createVariable(StorageClass storageClass, Id type, const char* name, Id initializer)
Id Builder::createVariable(Decoration precision, StorageClass storageClass, Id type, const char* name, Id initializer)
{
Id pointerType = makePointer(storageClass, type);
Instruction* inst = new Instruction(getUniqueId(), pointerType, OpVariable);
Expand All @@ -1381,6 +1381,7 @@ Id Builder::createVariable(StorageClass storageClass, Id type, const char* name,

if (name)
addName(inst->getResultId(), name);
setPrecision(inst->getResultId(), precision);

return inst->getResultId();
}
Expand Down Expand Up @@ -1437,7 +1438,8 @@ void Builder::createStore(Id rValue, Id lValue, spv::MemoryAccessMask memoryAcce
}

// Comments in header
Id Builder::createLoad(Id lValue, spv::MemoryAccessMask memoryAccess, spv::Scope scope, unsigned int alignment)
Id Builder::createLoad(Id lValue, spv::Decoration precision, spv::MemoryAccessMask memoryAccess,
spv::Scope scope, unsigned int alignment)
{
Instruction* load = new Instruction(getUniqueId(), getDerefTypeId(lValue), OpLoad);
load->addIdOperand(lValue);
Expand All @@ -1455,6 +1457,7 @@ Id Builder::createLoad(Id lValue, spv::MemoryAccessMask memoryAccess, spv::Scope
}

buildPoint->addInstruction(std::unique_ptr<Instruction>(load));
setPrecision(load->getResultId(), precision);

return load->getResultId();
}
Expand Down Expand Up @@ -2677,7 +2680,7 @@ void Builder::accessChainStore(Id rvalue, spv::MemoryAccessMask memoryAccess, sp
// If swizzle still exists, it is out-of-order or not full, we must load the target vector,
// extract and insert elements to perform writeMask and/or swizzle.
if (accessChain.swizzle.size() > 0) {
Id tempBaseId = createLoad(base);
Id tempBaseId = createLoad(base, spv::NoPrecision);
source = createLvalueSwizzle(getTypeId(tempBaseId), tempBaseId, source, accessChain.swizzle);
}

Expand Down Expand Up @@ -2716,17 +2719,17 @@ Id Builder::accessChainLoad(Decoration precision, Decoration nonUniform, Id resu

if (constant) {
id = createCompositeExtract(accessChain.base, swizzleBase, indexes);
setPrecision(id, precision);
} else {
Id lValue = NoResult;
if (spvVersion >= Spv_1_4) {
// make a new function variable for this r-value, using an initializer,
// and mark it as NonWritable so that downstream it can be detected as a lookup
// table
lValue = createVariable(StorageClassFunction, getTypeId(accessChain.base), "indexable",
accessChain.base);
lValue = createVariable(NoPrecision, StorageClassFunction, getTypeId(accessChain.base), "indexable", accessChain.base);
addDecoration(lValue, DecorationNonWritable);
} else {
lValue = createVariable(StorageClassFunction, getTypeId(accessChain.base), "indexable");
lValue = createVariable(NoPrecision, StorageClassFunction, getTypeId(accessChain.base), "indexable");
// store into it
createStore(accessChain.base, lValue);
}
Expand All @@ -2735,9 +2738,8 @@ Id Builder::accessChainLoad(Decoration precision, Decoration nonUniform, Id resu
accessChain.isRValue = false;

// load through the access chain
id = createLoad(collapseAccessChain());
id = createLoad(collapseAccessChain(), precision);
}
setPrecision(id, precision);
} else
id = accessChain.base; // no precision, it was set when this was defined
} else {
Expand All @@ -2756,8 +2758,7 @@ Id Builder::accessChainLoad(Decoration precision, Decoration nonUniform, Id resu
// loaded image types get decorated. TODO: This should maybe move to
// createImageTextureFunctionCall.
addDecoration(id, nonUniform);
id = createLoad(id, memoryAccess, scope, alignment);
setPrecision(id, precision);
id = createLoad(id, precision, memoryAccess, scope, alignment);
addDecoration(id, nonUniform);
}

Expand Down
6 changes: 4 additions & 2 deletions SPIRV/SpvBuilder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,8 @@ class Builder {
void makeDiscard();

// Create a global or function local or IO variable.
Id createVariable(StorageClass, Id type, const char* name = 0, Id initializer = NoResult);
Id createVariable(Decoration precision, StorageClass, Id type, const char* name = nullptr,
Id initializer = NoResult);

// Create an intermediate with an undefined value.
Id createUndefined(Id type);
Expand All @@ -357,7 +358,8 @@ class Builder {
spv::Scope scope = spv::ScopeMax, unsigned int alignment = 0);

// Load from an Id and return it
Id createLoad(Id lValue, spv::MemoryAccessMask memoryAccess = spv::MemoryAccessMaskNone,
Id createLoad(Id lValue, spv::Decoration precision,
spv::MemoryAccessMask memoryAccess = spv::MemoryAccessMaskNone,
spv::Scope scope = spv::ScopeMax, unsigned int alignment = 0);

// Create an OpAccessChain instruction
Expand Down
24 changes: 20 additions & 4 deletions SPIRV/spvIR.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -352,13 +352,27 @@ class Function {
const std::vector<Block*>& getBlocks() const { return blocks; }
void addLocalVariable(std::unique_ptr<Instruction> inst);
Id getReturnType() const { return functionInstruction.getTypeId(); }
void setReturnPrecision(Decoration precision)
{
if (precision == DecorationRelaxedPrecision)
reducedPrecisionReturn = true;
}
Decoration getReturnPrecision() const
{ return reducedPrecisionReturn ? DecorationRelaxedPrecision : NoPrecision; }

void setImplicitThis() { implicitThis = true; }
bool hasImplicitThis() const { return implicitThis; }

void addReducedPrecisionParam(int p) { reducedPrecisionParams.insert(p); }
bool isReducedPrecisionParam(int p) const
{ return reducedPrecisionParams.find(p) != reducedPrecisionParams.end(); }
void addParamPrecision(unsigned param, Decoration precision)
{
if (precision == DecorationRelaxedPrecision)
reducedPrecisionParams.insert(param);
}
Decoration getParamPrecision(unsigned param) const
{
return reducedPrecisionParams.find(param) != reducedPrecisionParams.end() ?
DecorationRelaxedPrecision : NoPrecision;
}

void dump(std::vector<unsigned int>& out) const
{
Expand All @@ -384,6 +398,7 @@ class Function {
std::vector<Instruction*> parameterInstructions;
std::vector<Block*> blocks;
bool implicitThis; // true if this is a member function expecting to be passed a 'this' as the first argument
bool reducedPrecisionReturn;
std::set<int> reducedPrecisionParams; // list of parameter indexes that need a relaxed precision arg
};

Expand Down Expand Up @@ -445,7 +460,8 @@ class Module {
// - the OpFunction instruction
// - all the OpFunctionParameter instructions
__inline Function::Function(Id id, Id resultType, Id functionType, Id firstParamId, Module& parent)
: parent(parent), functionInstruction(id, resultType, OpFunction), implicitThis(false)
: parent(parent), functionInstruction(id, resultType, OpFunction), implicitThis(false),
reducedPrecisionReturn(false)
{
// OpFunction
functionInstruction.addImmediateOperand(FunctionControlMaskNone);
Expand Down
52 changes: 31 additions & 21 deletions Test/baseResults/spv.forwardFun.frag.out
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
spv.forwardFun.frag
// Module Version 10000
// Generated by (magic number): 8000a
// Id's are bound by 60
// Id's are bound by 64

Capability Shader
1: ExtInstImport "GLSL.std.450"
MemoryModel Logical GLSL450
EntryPoint Fragment 4 "main" 20 30 36 59
EntryPoint Fragment 4 "main" 20 30 36 63
ExecutionMode 4 OriginUpperLeft
Source GLSL 140
Name 4 "main"
Expand All @@ -20,7 +20,7 @@ spv.forwardFun.frag
Name 27 "f"
Name 30 "gl_FragColor"
Name 36 "d"
Name 59 "bigColor"
Name 63 "bigColor"
Decorate 10(unreachableReturn() RelaxedPrecision
Decorate 16(foo(vf4;) RelaxedPrecision
Decorate 15(bar) RelaxedPrecision
Expand All @@ -39,10 +39,14 @@ spv.forwardFun.frag
Decorate 33 RelaxedPrecision
Decorate 36(d) RelaxedPrecision
Decorate 37 RelaxedPrecision
Decorate 52 RelaxedPrecision
Decorate 55 RelaxedPrecision
Decorate 44 RelaxedPrecision
Decorate 45 RelaxedPrecision
Decorate 49 RelaxedPrecision
Decorate 50 RelaxedPrecision
Decorate 56 RelaxedPrecision
Decorate 59(bigColor) RelaxedPrecision
Decorate 59 RelaxedPrecision
Decorate 60 RelaxedPrecision
Decorate 63(bigColor) RelaxedPrecision
2: TypeVoid
3: TypeFunction 2
8: TypeFloat 32
Expand All @@ -60,11 +64,11 @@ spv.forwardFun.frag
38: 8(float) Constant 1082549862
39: TypeBool
43: 8(float) Constant 1067030938
46: 8(float) Constant 1083179008
49: TypeInt 32 0
50: 49(int) Constant 0
53: 49(int) Constant 1
59(bigColor): 19(ptr) Variable Input
48: 8(float) Constant 1083179008
53: TypeInt 32 0
54: 53(int) Constant 0
57: 53(int) Constant 1
63(bigColor): 19(ptr) Variable Input
4(main): 2 Function None 3
5: Label
18(color): 13(ptr) Variable Function
Expand All @@ -90,25 +94,31 @@ spv.forwardFun.frag
FunctionEnd
10(unreachableReturn(): 8(float) Function None 9
11: Label
44: 26(ptr) Variable Function
49: 26(ptr) Variable Function
34: 2 FunctionCall 6(bar()
37: 8(float) Load 36(d)
40: 39(bool) FOrdLessThan 37 38
SelectionMerge 42 None
BranchConditional 40 41 45
BranchConditional 40 41 47
41: Label
ReturnValue 43
45: Label
ReturnValue 46
Store 44 43
45: 8(float) Load 44
ReturnValue 45
47: Label
Store 49 48
50: 8(float) Load 49
ReturnValue 50
42: Label
Unreachable
FunctionEnd
16(foo(vf4;): 8(float) Function None 14
15(bar): 13(ptr) FunctionParameter
17: Label
51: 26(ptr) AccessChain 15(bar) 50
52: 8(float) Load 51
54: 26(ptr) AccessChain 15(bar) 53
55: 8(float) Load 54
56: 8(float) FAdd 52 55
ReturnValue 56
55: 26(ptr) AccessChain 15(bar) 54
56: 8(float) Load 55
58: 26(ptr) AccessChain 15(bar) 57
59: 8(float) Load 58
60: 8(float) FAdd 56 59
ReturnValue 60
FunctionEnd
Loading

0 comments on commit 435dd80

Please sign in to comment.