Skip to content

Commit

Permalink
Avoid DXC warnings for missing bitwise op parantheses (shader-slang#4004
Browse files Browse the repository at this point in the history
)

Resolves shader-slang#3980

Based on the operator precedence, Slang may omits the parentheses if they
are not needed. DXC prints warnings for such cases and some applications
may treat the warnings as errors.

This commit emits parentheses to avoid the DXC warning even when they
are not needed.
  • Loading branch information
jkwak-work authored Apr 24, 2024
1 parent c6b9a91 commit 97631e9
Show file tree
Hide file tree
Showing 6 changed files with 265 additions and 8 deletions.
6 changes: 5 additions & 1 deletion prelude/slang-hlsl-prelude.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
#ifdef SLANG_HLSL_ENABLE_NVAPI
#include "nvHLSLExtns.h"
#endif
#pragma warning(disable: 3557)

#ifndef __DXC_VERSION_MAJOR
// warning X3557: loop doesn't seem to do anything, forcing loop to unroll
#pragma warning(disable: 3557)
#endif
37 changes: 33 additions & 4 deletions source/slang/slang-emit-c-like.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,35 @@ bool CLikeSourceEmitter::maybeEmitParens(EmitOpInfo& outerPrec, const EmitOpInfo
bool needParens = (prec.leftPrecedence <= outerPrec.leftPrecedence)
|| (prec.rightPrecedence <= outerPrec.rightPrecedence);

// While Slang correctly removes some of parentheses, DXC prints warnings
// for common mistakes when parentheses are not used with certain combinations
// of the operations. We emit parentheses to avoid the warnings.
//
// a | b & c => a | (b & c)
if (prec.leftPrecedence == EPrecedence::kEPrecedence_BitAnd_Left
&& outerPrec.leftPrecedence == EPrecedence::kEPrecedence_BitOr_Right)
{
needParens = true;
}
// a & b | c => (a & b) | c
else if (prec.rightPrecedence == EPrecedence::kEPrecedence_BitAnd_Right
&& outerPrec.rightPrecedence == EPrecedence::kEPrecedence_BitOr_Left)
{
needParens = true;
}
// a << b + c => a << (b + c)
else if (prec.leftPrecedence == EPrecedence::kEPrecedence_Additive_Left
&& outerPrec.leftPrecedence == EPrecedence::kEPrecedence_Shift_Right)
{
needParens = true;
}
// a + b << c => (a + b) << c
else if (prec.rightPrecedence == EPrecedence::kEPrecedence_Additive_Right
&& outerPrec.rightPrecedence == EPrecedence::kEPrecedence_Shift_Left)
{
needParens = true;
}

if (needParens)
{
m_writer->emit("(");
Expand Down Expand Up @@ -2305,7 +2334,7 @@ void CLikeSourceEmitter::defaultEmitInstExpr(IRInst* inst, const EmitOpInfo& inO
}
}

emitOperand(operand, rightSide(prec, outerPrec));
emitOperand(operand, rightSide(outerPrec, prec));
break;
}
case kIROp_Load:
Expand Down Expand Up @@ -2455,7 +2484,7 @@ void CLikeSourceEmitter::defaultEmitInstExpr(IRInst* inst, const EmitOpInfo& inO
needClose = maybeEmitParens(outerPrec, prec);
emitOperand(inst->getOperand(0), leftSide(outerPrec, prec));
m_writer->emit(" + ");
emitOperand(inst->getOperand(1), rightSide(prec, outerPrec));
emitOperand(inst->getOperand(1), rightSide(outerPrec, prec));
break;
}
case kIROp_GetElement:
Expand All @@ -2472,7 +2501,7 @@ void CLikeSourceEmitter::defaultEmitInstExpr(IRInst* inst, const EmitOpInfo& inO
m_writer->emit("[");
emitOperand(inst->getOperand(1), getInfo(EmitOp::General));
m_writer->emit("].");
emitOperand(inst->getOperand(0), rightSide(prec, outerPrec));
emitOperand(inst->getOperand(0), rightSide(outerPrec, prec));
break;
}
else
Expand Down Expand Up @@ -2558,7 +2587,7 @@ void CLikeSourceEmitter::defaultEmitInstExpr(IRInst* inst, const EmitOpInfo& inO
m_writer->emit(" ? ");
emitOperand(inst->getOperand(1), prec);
m_writer->emit(" : ");
emitOperand(inst->getOperand(2), rightSide(prec, outerPrec));
emitOperand(inst->getOperand(2), rightSide(outerPrec, prec));
}
break;

Expand Down
2 changes: 1 addition & 1 deletion source/slang/slang-emit-glsl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2064,7 +2064,7 @@ bool GLSLSourceEmitter::tryEmitInstExprImpl(IRInst* inst, const EmitOpInfo& inOu
}

m_writer->emit(" = ");
emitOperand(inst->getOperand(2), rightSide(assignPrec, outerPrec));
emitOperand(inst->getOperand(2), rightSide(outerPrec, assignPrec));
maybeCloseParens(assignNeedsClose);
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion source/slang/slang-emit-metal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ bool MetalSourceEmitter::tryEmitInstExprImpl(IRInst* inst, const EmitOpInfo& inO
needClose = maybeEmitParens(outerPrec, prec);
emitOperand(inst->getOperand(0), leftSide(outerPrec, prec));
m_writer->emit("+");
emitOperand(inst->getOperand(1), rightSide(prec, outerPrec));
emitOperand(inst->getOperand(1), rightSide(outerPrec, prec));
maybeCloseParens(needClose);
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion source/slang/slang-emit-precedence.h
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ SLANG_INLINE EmitOpInfo leftSide(EmitOpInfo const& outerPrec, EmitOpInfo const&
return result;
}

SLANG_INLINE EmitOpInfo rightSide(EmitOpInfo const& prec, EmitOpInfo const& outerPrec)
SLANG_INLINE EmitOpInfo rightSide(EmitOpInfo const& outerPrec, EmitOpInfo const& prec)
{
EmitOpInfo result;
result.op = nullptr;
Expand Down
224 changes: 224 additions & 0 deletions tests/bugs/gh-3980.slang
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
//TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=BUF):-slang -compute -output-using-type
//TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=BUF):-slang -compute -output-using-type -dx12 -profile cs_6_6 -use-dxil
//TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=BUF):-vk -compute -shaderobj -output-using-type
//TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=BUF):-vk -compute -shaderobj -output-using-type -emit-spirv-directly
//TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=BUF):-cpu -compute -output-using-type
//TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=BUF):-cuda -compute -output-using-type

// Slang removes parentheses characters for the bitwise operators when they are not needed.
// DXC prints warning messages even when the expression is correct.
// We are snoozing the warning and testing it here to prevent a regression.

//TEST_INPUT: ubuffer(data=[0 1 2 3 4 5 6 7 8 9], stride=4):name inputBuffer
RWStructuredBuffer<int> inputBuffer;

//TEST_INPUT: ubuffer(data=[0 0 0 0], stride=4):out,name outputBuffer
RWStructuredBuffer<int> outputBuffer;

// -----------+----------+------------------------------+---------------
// Precedence | Operator | Description | Associativity
// -----------+----------+------------------------------+---------------
// 4 | + - | Addition and subtraction | Left-to-right
// -----------+----------+------------------------------+
// 5 | << >> | Bitwise left and right shift |
// -----------+----------+------------------------------+
// 8 | & | Bitwise AND |
// -----------+----------+------------------------------+
// 9 | ^ | Bitwise XOR (exclusive or) |
// -----------+----------+------------------------------+
// 10 | | | Bitwise OR (inclusive or) |
// -----------+----------+------------------------------+---------------

bool Test_And_Or()
{
uint32_t a = inputBuffer[1];
uint32_t b = inputBuffer[3];
uint32_t c = inputBuffer[2];
uint32_t d = inputBuffer[6];

return true
&& 3 == ((a & b) | (c & d))
&& 0 == (a & (b | c) & d)
&& 2 == (((a & b) | c) & d)
&& 1 == (a & (b | (c & d)))
&& 3 == (a & b | c & d)

&& 2 == ((a | b) & (c | d))
&& 7 == (a | (b & c) | d)
&& 6 == (((a | b) & c) | d)
&& 3 == (a | (b & (c | d)))
&& 7 == (a | b & c | d)
;
}

bool Test_And_Xor()
{
uint32_t a = inputBuffer[1];
uint32_t b = inputBuffer[3];
uint32_t c = inputBuffer[2];
uint32_t d = inputBuffer[6];

return true
&& 3 == ((a & b) ^ (c & d))
&& 0 == (a & (b ^ c) & d)
&& 2 == (((a & b) ^ c) & d)
&& 1 == (a & (b ^ (c & d)))
&& 3 == (a & b ^ c & d)

&& 0 == ((a ^ b) & (c ^ d))
&& 5 == (a ^ (b & c) ^ d)
&& 4 == (((a ^ b) & c) ^ d)
&& 1 == (a ^ (b & (c ^ d)))
&& 5 == (a ^ b & c ^ d)
;
}

bool Test_Xor_Or()
{
uint32_t a = inputBuffer[1];
uint32_t b = inputBuffer[4];
uint32_t c = inputBuffer[2];
uint32_t d = inputBuffer[7];

return true
&& 5 == ((a ^ b) | (c ^ d))
&& 0 == (a ^ (b | c) ^ d)
&& 0 == (((a ^ b) | c) ^ d)
&& 4 == (a ^ (b | (c ^ d)))
&& 5 == (a ^ b | c ^ d)

&& 2 == ((a | b) ^ (c | d))
&& 7 == (a | (b ^ c) | d)
&& 7 == (((a | b) ^ c) | d)
&& 3 == (a | (b ^ (c | d)))
&& 7 == (a | b ^ c | d)
;
}

bool Test_LShift_RShift()
{
uint32_t a = inputBuffer[4];
uint32_t b = inputBuffer[1];
uint32_t c = inputBuffer[2];
uint32_t d = inputBuffer[3];

return true
&& 0 == ((a << b) >> (c << d))
&& 32 == (a << (b >> c) << d)
&& 16 == (((a << b) >> c) << d)
&& 4 == (a << (b >> (c << d)))
&& 16 == (a << b >> c << d)

&& 2 == ((a >> b) << (c >> d))
&& 0 == (a >> (b << c) >> d)
&& 1 == (((a >> b) << c) >> d)
&& 2 == (a >> (b << (c >> d)))
&& 1 == (a >> b << c >> d)
;
}

bool Test_LShift_And()
{
uint32_t a = inputBuffer[1];
uint32_t b = inputBuffer[5];
uint32_t c = inputBuffer[2];
uint32_t d = inputBuffer[4];

return true
&& 32 == ((a << b) & (c << d))
&& 16 == (a << (b & c) << d)
&& 0 == (((a << b) & c) << d)
&& 1 == (a << (b & (c << d)))
&& 32 == (a << b & c << d)

&& 1 == ((a & b) << (c & d))
&& 0 == (a & (b << c) & d)
&& 4 == (((a & b) << c) & d)
&& 1 == (a & (b << (c & d)))
&& 0 == (a & b << c & d)
;
}

bool Test_LShift_Or()
{
uint32_t a = inputBuffer[1];
uint32_t b = inputBuffer[2];
uint32_t c = inputBuffer[4];
uint32_t d = inputBuffer[3];

return true
&& 36 == ((a << b) | (c << d))
&& 512 == (a << (b | c) << d)
&& 32 == (((a << b) | c) << d)
&& 1073741824 == (a << ((b | (c << d)) - 4))
&& 36 == (a << b | c << d)

&& 384 == ((a | b) << (c | d))
&& 35 == (a | (b << c) | d)
&& 51 == (((a | b) << c) | d)
&& 257 == (a | (b << (c | d)))
&& 35 == (a | b << c | d)
;
}

bool Test_LShift_Xor()
{
uint32_t a = inputBuffer[1];
uint32_t b = inputBuffer[2];
uint32_t c = inputBuffer[4];
uint32_t d = inputBuffer[3];

return true
&& 36 == ((a << b) ^ (c << d))
&& 512 == (a << (b ^ c) << d)
&& 0 == (((a << b) ^ c) << d)
&& 1073741824 == ((a << ((b ^ (c << d)) - 4)))
&& 36 == (a << b ^ c << d)

&& 384 == ((a ^ b) << (c ^ d))
&& 34 == (a ^ (b << c) ^ d)
&& 51 == (((a ^ b) << c) ^ d)
&& 257 == (a ^ (b << (c ^ d)))
&& 34 == (a ^ b << c ^ d)
;
}

bool Test_Add_LShift()
{
uint32_t a = inputBuffer[1];
uint32_t b = inputBuffer[3];
uint32_t c = inputBuffer[2];
uint32_t d = inputBuffer[4];

return true
&& 256 == ((a + b) << (c + d))
&& 17 == (a + (b << c) + d)
&& 20 == (((a + b) << c) + d)
&& 193 == (a + (b << (c + d)))
&& 256 == (a + b << c + d)

&& 40 == ((a << b) + (c << d))
&& 512 == (a << (b + c) << d)
&& 160 == (((a << b) + c) << d)
&& 1073741824 == (a << ((b + (c << d)) - 5))
&& 512 == (a << b + c << d)
;
}

[numthreads(1, 1, 1)]
void computeMain(uint groupIndex : SV_GroupIndex, int3 dispatchThreadID: SV_DispatchThreadID)
{
//BUF: 1
bool result = true
&& Test_And_Or()
&& Test_And_Xor()
&& Test_Xor_Or()
&& Test_LShift_RShift()
&& Test_LShift_And()
&& Test_LShift_Or()
&& Test_LShift_Xor()
&& Test_Add_LShift()
;

outputBuffer[0] = int(result);
}

0 comments on commit 97631e9

Please sign in to comment.