HLSL: Fix #1249: Always execute both sides of ternary "?:".
This is semantically required by HLSL, and frequently results in using
OpSelect instead of control flow.
diff --git a/SPIRV/GlslangToSpv.cpp b/SPIRV/GlslangToSpv.cpp
index dc644e5..7c69242 100755
--- a/SPIRV/GlslangToSpv.cpp
+++ b/SPIRV/GlslangToSpv.cpp
@@ -1973,18 +1973,29 @@
// next layer copies r-values into memory to use the access-chain mechanism
bool TGlslangToSpvTraverser::visitSelection(glslang::TVisit /* visit */, glslang::TIntermSelection* node)
{
- // See if it simple and safe to generate OpSelect instead of using control flow.
- // Crucially, side effects must be avoided, and there are performance trade-offs.
- // Return true if good idea (and safe) for OpSelect, false otherwise.
- const auto selectPolicy = [&]() -> bool {
- if ((!node->getType().isScalar() && !node->getType().isVector()) ||
- node->getBasicType() == glslang::EbtVoid)
- return false;
-
+ // See if it simple and safe, or required, to execute both sides.
+ // Crucially, side effects must be either semantically required or avoided,
+ // and there are performance trade-offs.
+ // Return true if required or a good idea (and safe) to execute both sides,
+ // false otherwise.
+ const auto bothSidesPolicy = [&]() -> bool {
+ // do we have both sides?
if (node->getTrueBlock() == nullptr ||
node->getFalseBlock() == nullptr)
return false;
+ // required? (unless we write additional code to look for side effects
+ // and make performance trade-offs if none are present)
+ if (!node->getShortCircuit())
+ return true;
+
+ // if not required to execute both, decide based on performance/practicality...
+
+ // see if OpSelect can handle it
+ if ((!node->getType().isScalar() && !node->getType().isVector()) ||
+ node->getBasicType() == glslang::EbtVoid)
+ return false;
+
assert(node->getType() == node->getTrueBlock() ->getAsTyped()->getType() &&
node->getType() == node->getFalseBlock()->getAsTyped()->getType());
@@ -1997,10 +2008,14 @@
operandOkay(node->getFalseBlock()->getAsTyped());
};
- // Emit OpSelect for this selection.
- const auto handleAsOpSelect = [&]() {
- node->getCondition()->traverse(this);
- spv::Id condition = accessChainLoad(node->getCondition()->getType());
+ spv::Id result = spv::NoResult; // upcoming result selecting between trueValue and falseValue
+ // emit the condition before doing anything with selection
+ node->getCondition()->traverse(this);
+ spv::Id condition = accessChainLoad(node->getCondition()->getType());
+
+ // Find a way of executing both sides and selecting the right result.
+ const auto executeBothSides = [&]() -> void {
+ // execute both sides
node->getTrueBlock()->traverse(this);
spv::Id trueValue = accessChainLoad(node->getTrueBlock()->getAsTyped()->getType());
node->getFalseBlock()->traverse(this);
@@ -2008,72 +2023,98 @@
builder.setLine(node->getLoc().line);
- // smear condition to vector, if necessary (AST is always scalar)
- if (builder.isVector(trueValue))
- condition = builder.smearScalar(spv::NoPrecision, condition,
- builder.makeVectorType(builder.makeBoolType(),
- builder.getNumComponents(trueValue)));
+ // done if void
+ if (node->getBasicType() == glslang::EbtVoid)
+ return;
- spv::Id select = builder.createTriOp(spv::OpSelect,
- convertGlslangToSpvType(node->getType()), condition,
- trueValue, falseValue);
- builder.clearAccessChain();
- builder.setAccessChainRValue(select);
+ // emit code to select between trueValue and falseValue
+
+ // see if OpSelect can handle it
+ if (node->getType().isScalar() || node->getType().isVector()) {
+ // Emit OpSelect for this selection.
+
+ // smear condition to vector, if necessary (AST is always scalar)
+ if (builder.isVector(trueValue))
+ condition = builder.smearScalar(spv::NoPrecision, condition,
+ builder.makeVectorType(builder.makeBoolType(),
+ builder.getNumComponents(trueValue)));
+
+ // OpSelect
+ result = builder.createTriOp(spv::OpSelect,
+ convertGlslangToSpvType(node->getType()), condition,
+ trueValue, falseValue);
+
+ builder.clearAccessChain();
+ builder.setAccessChainRValue(result);
+ } 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()));
+
+ // Selection control:
+ const spv::SelectionControlMask control = TranslateSelectionControl(*node);
+
+ // make an "if" based on the value created by the condition
+ spv::Builder::If ifBuilder(condition, control, builder);
+
+ // emit the "then" statement
+ builder.createStore(trueValue, result);
+ ifBuilder.makeBeginElse();
+ // emit the "else" statement
+ builder.createStore(falseValue, result);
+
+ // finish off the control flow
+ ifBuilder.makeEndIf();
+
+ builder.clearAccessChain();
+ builder.setAccessChainLValue(result);
+ }
};
- // Try for OpSelect
+ // 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 (selectPolicy()) {
+ // Selection control:
+ const spv::SelectionControlMask control = TranslateSelectionControl(*node);
+
+ // make an "if" based on the value created by the condition
+ spv::Builder::If ifBuilder(condition, control, builder);
+
+ // emit the "then" statement
+ if (node->getTrueBlock() != nullptr) {
+ node->getTrueBlock()->traverse(this);
+ if (result != spv::NoResult)
+ builder.createStore(accessChainLoad(node->getTrueBlock()->getAsTyped()->getType()), result);
+ }
+
+ if (node->getFalseBlock() != nullptr) {
+ ifBuilder.makeBeginElse();
+ // emit the "else" statement
+ node->getFalseBlock()->traverse(this);
+ if (result != spv::NoResult)
+ builder.createStore(accessChainLoad(node->getFalseBlock()->getAsTyped()->getType()), result);
+ }
+
+ // finish off the control flow
+ ifBuilder.makeEndIf();
+
+ if (result != spv::NoResult) {
+ builder.clearAccessChain();
+ builder.setAccessChainLValue(result);
+ }
+ };
+
+ // Try for OpSelect (or a requirement to execute both sides)
+ if (bothSidesPolicy()) {
SpecConstantOpModeGuard spec_constant_op_mode_setter(&builder);
if (node->getType().getQualifier().isSpecConstant())
spec_constant_op_mode_setter.turnOnSpecConstantOpMode();
-
- handleAsOpSelect();
- return false;
- }
-
- // Instead, emit control flow...
- // Don't handle results as temporaries, because there will be two names
- // and better to leave SSA to later passes.
- spv::Id result = (node->getBasicType() == glslang::EbtVoid)
- ? spv::NoResult
- : builder.createVariable(spv::StorageClassFunction, convertGlslangToSpvType(node->getType()));
-
- // emit the condition before doing anything with selection
- node->getCondition()->traverse(this);
-
- // Selection control:
- const spv::SelectionControlMask control = TranslateSelectionControl(*node);
-
- // make an "if" based on the value created by the condition
- spv::Builder::If ifBuilder(accessChainLoad(node->getCondition()->getType()), control, builder);
-
- // emit the "then" statement
- if (node->getTrueBlock() != nullptr) {
- node->getTrueBlock()->traverse(this);
- if (result != spv::NoResult)
- builder.createStore(accessChainLoad(node->getTrueBlock()->getAsTyped()->getType()), result);
- }
-
- if (node->getFalseBlock() != nullptr) {
- ifBuilder.makeBeginElse();
- // emit the "else" statement
- node->getFalseBlock()->traverse(this);
- if (result != spv::NoResult)
- builder.createStore(accessChainLoad(node->getFalseBlock()->getAsTyped()->getType()), result);
- }
-
- // finish off the control flow
- ifBuilder.makeEndIf();
-
- if (result != spv::NoResult) {
- // GLSL only has r-values as the result of a :?, but
- // if we have an l-value, that can be more efficient if it will
- // become the base of a complex r-value expression, because the
- // next layer copies r-values into memory to use the access-chain mechanism
- builder.clearAccessChain();
- builder.setAccessChainLValue(result);
- }
+ executeBothSides();
+ } else
+ executeOneSide();
return false;
}