Improve GLSL coverage of boolean binary ops (#1335)

* Improve GLSL coverage of boolean binary ops

This change ensures that the `&&`, `||`, `&`, `|`, and `^` apply correctly to vectors of `bool` values when targetting GLSL.

Most of the changes are in the GLSL emit path, where the IR instructions for these operators are bottlenecked through a small set of helper routines to cover the different cases. In general:

* The vector variants of the operations are implemented by casting to `uint` vectors, performing bitwise ops, then casting back
* The scalar variants are handled by conveting the bitwise operations to their equivalent logical operator (the one interesting case there is bitwise `^` where the equivalent logical operation on `bool` is `!=`)

This change makes it clear that our IR really shouldn't have distinct opcodes for logical vs. bitwise and/or/xor, and instead should just have a single family of operations where the behavior differs based on the type of the operand. That is already *de facto* the way things work (a user can always write `&`, `|` and `^` and expect them to work on `bool` and vectors of `bool`), so that the GLSL output path has to deal with the overlap. Having two sets of IR ops here actually makes for more code instead of less.

* Fixups: review feedback and test ! operator

2 files changed, 140 insertions, 35 deletions

diff --git a/source/slang/slang-emit-glsl.cpp b/source/slang/slang-emit-glsl.cpp
index 251b164cd..00ce929d9 100644
--- a/source/slang/slang-emit-glsl.cpp
+++ b/source/slang/slang-emit-glsl.cpp
@@ -1018,16 +1018,6 @@ void GLSLSourceEmitter::emitLayoutQualifiersImpl(IRVarLayout* layout)
     }
 }
 
-static EmitOp _getBoolOp(IROp op)
-{
-    switch (op)
-    {
-        case kIROp_BitAnd:          return EmitOp::And;
-        case kIROp_BitOr:           return EmitOp::Or;
-        default:                    return EmitOp::None;
-    }
-}
-
 static const char* _getGLSLVectorCompareFunctionName(IROp op)
 {
     // Glsl vector comparisons use functions...
@@ -1065,6 +1055,102 @@ void GLSLSourceEmitter::_maybeEmitGLSLCast(IRType* castType, IRInst* inst)
     }
 }
 
+void GLSLSourceEmitter::_emitLegalizedBoolVectorBinOp(IRInst* inst, IRVectorType* type, const EmitOpInfo& op, const EmitOpInfo& inOuterPrec)
+{
+    auto elementCount = type->getElementCount();
+
+    EmitOpInfo outerPrec = inOuterPrec;
+    auto prec = getInfo(EmitOp::Postfix);
+    bool needClose = maybeEmitParens(outerPrec, prec);
+
+    emitType(type);
+    m_writer->emit("(uvec");
+    emitSimpleValue(elementCount);
+    m_writer->emit("(");
+    emitOperand(inst->getOperand(0), getInfo(EmitOp::General));
+    m_writer->emit(")");
+    m_writer->emit(op.op);
+    m_writer->emit("uvec");
+    emitSimpleValue(elementCount);
+    m_writer->emit("(");
+    emitOperand(inst->getOperand(1), getInfo(EmitOp::General));
+    m_writer->emit("))");
+
+    maybeCloseParens(needClose);
+}
+
+bool GLSLSourceEmitter::_tryEmitLogicalBinOp(IRInst* inst, const EmitOpInfo& bitOp, const EmitOpInfo& inOuterPrec)
+{
+    // Logical operation on scalar `bool` values are directly
+    // supported by GLSL. They have short-circuiting behavior,
+    // but we need not worry about that because our logic
+    // for folding sub-expressions into their use sites will
+    // never fold a sub-expression that would have side effects.
+    //
+    // Thus we fall back to the default handling for scalar
+    // cases (which should only arise for `bool` operands).
+    //
+    IRType* type = inst->getDataType();
+    auto vectorType = as<IRVectorType>(type);
+    if(!vectorType)
+        return false;
+
+    // For vector cases, we need to convert the operands to
+    // a type that supports vector operations, and then use
+    // bit operations there.
+    //
+    _emitLegalizedBoolVectorBinOp(inst, vectorType, bitOp, inOuterPrec);
+    return true;
+}
+
+bool GLSLSourceEmitter::_tryEmitBitBinOp(IRInst* inst, const EmitOpInfo& bitOp, const EmitOpInfo& boolOp, const EmitOpInfo& inOuterPrec)
+{
+    // The bitwise binary operations are supported in GLSL,
+    // but do not support `bool` or vector-of-`bool` operands.
+    //
+    // We start by checking if we have a `bool`-based case,
+    // and fall back to the default emit logic if not.
+    //
+    IRType* type = inst->getDataType();
+    IRType* elementType = type;
+    auto vectorType = as<IRVectorType>(type);
+    if(vectorType)
+        elementType = vectorType->getElementType();
+    if(!as<IRBoolType>(elementType))
+        return false;
+
+    // If we have a vector case, then it will be handled
+    // by casting the `bool` vectors to vectors of
+    // integers and doing the bitwise op there, where
+    // it should yield an equivalent result.
+    //
+    if(vectorType)
+    {
+        _emitLegalizedBoolVectorBinOp(inst, vectorType, bitOp, inOuterPrec);
+    }
+    else
+    {
+        // In the scalar case, we will translate
+        // bitwise operations on `bool` values to
+        // the equivalent logical operation, knowing
+        // that our appraoch to folding of sub-expressions
+        // into use sites will avoid any potential issues
+        // around short-circuiting behavior.
+        //
+        auto prec = boolOp;
+        EmitOpInfo outerPrec = inOuterPrec;
+        bool needClose = maybeEmitParens(outerPrec, prec);
+
+        emitOperand(inst->getOperand(0), leftSide(outerPrec, prec));
+        m_writer->emit(prec.op);
+        emitOperand(inst->getOperand(1), rightSide(outerPrec, prec));
+
+        maybeCloseParens(needClose);
+    }
+    return true;
+
+}
+
 bool GLSLSourceEmitter::tryEmitInstExprImpl(IRInst* inst, const EmitOpInfo& inOuterPrec)
 {
     switch (inst->op)
@@ -1149,6 +1235,10 @@ bool GLSLSourceEmitter::tryEmitInstExprImpl(IRInst* inst, const EmitOpInfo& inOu
 
             return true;
         }
+        case kIROp_And:
+            return _tryEmitLogicalBinOp(inst, getInfo(EmitOp::BitAnd), inOuterPrec);
+        case kIROp_Or:
+            return _tryEmitLogicalBinOp(inst, getInfo(EmitOp::BitOr), inOuterPrec);
         case kIROp_Not:
         {
             IRInst* operand = inst->getOperand(0);
@@ -1170,33 +1260,24 @@ bool GLSLSourceEmitter::tryEmitInstExprImpl(IRInst* inst, const EmitOpInfo& inOu
             }
             return false;
         }
+
+        // When emitting a bitwise operation in GLSL, we need to special-case the handling
+        // of `bool` and vectors of `bool` so that they produce valid results by operating
+        // on the single-bit truth value.
+        //
+        // In the case of a vector we will convert to `uint` vectors and perform the
+        // bitwise op on them before converting back to `bool` vectors.
+        //
+        // In the scalar case we will apply the corresponding logical operation to
+        // the `bool` operands.
+        //
         case kIROp_BitAnd:
+            return _tryEmitBitBinOp(inst, getInfo(EmitOp::BitAnd), getInfo(EmitOp::And), inOuterPrec);
         case kIROp_BitOr:
-        {
-            // Are we targetting GLSL, and are both operands scalar bools?
-            // In that case convert the operation to a logical And
-            if (as<IRBoolType>(inst->getOperand(0)->getDataType())
-                && as<IRBoolType>(inst->getOperand(1)->getDataType()))
-            {
-                EmitOpInfo outerPrec = inOuterPrec;
-                bool needClose = maybeEmitParens(outerPrec, outerPrec);
-
-                // Get the boolean version of the op
-                const auto op = _getBoolOp(inst->op);
-                auto prec = getInfo(op);
-
-                // TODO: handle a bitwise Or of a vector of bools by casting to
-                // a uvec and performing the bitwise operation
-
-                emitOperand(inst->getOperand(0), leftSide(outerPrec, prec));
-                m_writer->emit(prec.op);
-                emitOperand(inst->getOperand(1), rightSide(outerPrec, prec));
-
-                maybeCloseParens(needClose);
-                return true;
-            }
-            break;
-        }
+            return _tryEmitBitBinOp(inst, getInfo(EmitOp::BitOr), getInfo(EmitOp::Or), inOuterPrec);
+        case kIROp_BitXor:
+            // Note: on scalar `bool` operands, a bitwise XOR (`^`) is equivalent to a not-equal (`!=`) comparison.
+            return _tryEmitBitBinOp(inst, getInfo(EmitOp::BitXor), getInfo(EmitOp::Neq), inOuterPrec);
 
         // Comparisons
         case kIROp_Eql:
diff --git a/source/slang/slang-emit-glsl.h b/source/slang/slang-emit-glsl.h
index bc430c9a8..760d62194 100644
--- a/source/slang/slang-emit-glsl.h
+++ b/source/slang/slang-emit-glsl.h
@@ -75,6 +75,30 @@ protected:
 
     void _maybeEmitGLSLCast(IRType* castType, IRInst* inst);
 
+        /// Emit the legalized form of a bitwise or logical operation on a vector of `bool`.
+        ///
+        /// This emits GLSL code that converts the operands of `inst` into vectors of
+        /// `uint`, then applies `op` bitwise to the result, and finally converts back
+        /// into the desired vector-of-bool `type`.
+        ///
+    void _emitLegalizedBoolVectorBinOp(IRInst* inst, IRVectorType* type, const EmitOpInfo& op, const EmitOpInfo& inOuterPrec);
+
+        /// Try to emit specialized code for a logic binary op.
+        ///
+        /// Returns true if specialized code was emitted, false if the default behavior should be used.
+        ///
+        /// The `bitOp` parameter should be the bitwise equivalent of the logical op being emitted.
+        ///
+    bool _tryEmitLogicalBinOp(IRInst* inst, const EmitOpInfo& bitOp, const EmitOpInfo& inOuterPrec);
+
+        /// Try to emit specialized code for a logic binary op.
+        ///
+        /// Returns true if specialized code was emitted, false if the default behavior should be used.
+        ///
+        /// The `bitOp` parameter should be the bitwise op being emitted.
+        /// The `bitOp` parameter should be the logical equivalent of `bitOp`
+        ///
+    bool _tryEmitBitBinOp(IRInst* inst, const EmitOpInfo& bitOp, const EmitOpInfo& boolOp, const EmitOpInfo& inOuterPrec);
 
     RefPtr<GLSLExtensionTracker> m_glslExtensionTracker;
 };