From 04562dea2681c8fea9f2f3472e0700599ed57d44 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Thu, 12 Oct 2023 13:58:56 +0200 Subject: [PATCH] Split `UnaryOperator::Not` into `UnaryOperator::LogicalNot` & `UnaryOperator::BitwiseNot` since it should not be valid to use the logical and bitwise not operators interchangeably also, don't allow `UnaryOperator::Negate` to operate on booleans (no frontend/backend supports this) --- src/back/glsl/mod.rs | 40 +++++++--------------------- src/back/hlsl/writer.rs | 18 +++---------- src/back/msl/writer.rs | 10 +++---- src/back/spv/block.rs | 12 +++------ src/back/wgsl/writer.rs | 9 ++----- src/front/glsl/builtins.rs | 2 +- src/front/glsl/parser/expressions.rs | 8 ++++-- src/front/glsl/parser/functions.rs | 6 ++--- src/front/spv/mod.rs | 6 ++--- src/front/wgsl/parse/mod.rs | 14 ++++++++-- src/lib.rs | 3 ++- src/proc/constant_evaluator.rs | 11 +++++--- src/valid/expression.rs | 7 +++-- 13 files changed, 58 insertions(+), 88 deletions(-) diff --git a/src/back/glsl/mod.rs b/src/back/glsl/mod.rs index 77e206d3c6..a4d0839d4c 100644 --- a/src/back/glsl/mod.rs +++ b/src/back/glsl/mod.rs @@ -2762,38 +2762,18 @@ impl<'a, W: Write> Writer<'a, W> { write!(self.out, ")")?; } - // `Unary` is pretty straightforward - // "-" - for `Negate` - // "~" - for `Not` if it's an integer - // "!" - for `Not` if it's a boolean - // - // We also wrap the everything in parentheses to avoid precedence issues Expression::Unary { op, expr } => { - use crate::{ScalarKind as Sk, UnaryOperator as Uo}; - - let ty = ctx.info[expr].ty.inner_with(&self.module.types); - - match *ty { - TypeInner::Vector { kind: Sk::Bool, .. } => { - write!(self.out, "not(")?; - } - _ => { - let operator = match op { - Uo::Negate => "-", - Uo::Not => match ty.scalar_kind() { - Some(Sk::Sint) | Some(Sk::Uint) => "~", - Some(Sk::Bool) => "!", - ref other => { - return Err(Error::Custom(format!( - "Cannot apply not to type {other:?}" - ))) - } - }, - }; - - write!(self.out, "{operator}(")?; + let operator_or_fn = match op { + crate::UnaryOperator::Negate => "-", + crate::UnaryOperator::LogicalNot => { + match *ctx.info[expr].ty.inner_with(&self.module.types) { + TypeInner::Vector { .. } => "not", + _ => "!", + } } - } + crate::UnaryOperator::BitwiseNot => "~", + }; + write!(self.out, "{operator_or_fn}(")?; self.write_expr(expr, ctx)?; diff --git a/src/back/hlsl/writer.rs b/src/back/hlsl/writer.rs index 590850fc4d..b8581e5e74 100644 --- a/src/back/hlsl/writer.rs +++ b/src/back/hlsl/writer.rs @@ -2566,23 +2566,11 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> { } } Expression::Unary { op, expr } => { - use crate::{ScalarKind as Sk, UnaryOperator as Uo}; // https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-operators#unary-operators let op_str = match op { - Uo::Negate => "-", - Uo::Not => match func_ctx.info[expr] - .ty - .inner_with(&module.types) - .scalar_kind() - { - Some(Sk::Sint) | Some(Sk::Uint) => "~", - Some(Sk::Bool) => "!", - ref other => { - return Err(Error::Custom(format!( - "Cannot apply not to type {other:?}" - ))) - } - }, + crate::UnaryOperator::Negate => "-", + crate::UnaryOperator::LogicalNot => "!", + crate::UnaryOperator::BitwiseNot => "~", }; write!(self.out, "{op_str}(")?; self.write_expr(module, expr, func_ctx)?; diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index d0f5413136..c0c19aa49a 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -1551,14 +1551,10 @@ impl Writer { } }, crate::Expression::Unary { op, expr } => { - use crate::{ScalarKind as Sk, UnaryOperator as Uo}; let op_str = match op { - Uo::Negate => "-", - Uo::Not => match context.resolve_type(expr).scalar_kind() { - Some(Sk::Sint) | Some(Sk::Uint) => "~", - Some(Sk::Bool) => "!", - _ => return Err(Error::Validation), - }, + crate::UnaryOperator::Negate => "-", + crate::UnaryOperator::LogicalNot => "!", + crate::UnaryOperator::BitwiseNot => "~", }; write!(self.out, "{op_str}(")?; self.put_expression(expr, context, false)?; diff --git a/src/back/spv/block.rs b/src/back/spv/block.rs index 4dba7ea0ca..0e3a7c16e9 100644 --- a/src/back/spv/block.rs +++ b/src/back/spv/block.rs @@ -463,16 +463,10 @@ impl<'w> BlockContext<'w> { crate::UnaryOperator::Negate => match expr_ty_inner.scalar_kind() { Some(crate::ScalarKind::Float) => spirv::Op::FNegate, Some(crate::ScalarKind::Sint) => spirv::Op::SNegate, - Some(crate::ScalarKind::Bool) => spirv::Op::LogicalNot, - Some(crate::ScalarKind::Uint) | None => { - log::error!("Unable to negate {:?}", expr_ty_inner); - return Err(Error::FeatureNotImplemented("negation")); - } - }, - crate::UnaryOperator::Not => match expr_ty_inner.scalar_kind() { - Some(crate::ScalarKind::Bool) => spirv::Op::LogicalNot, - _ => spirv::Op::Not, + _ => return Err(Error::Validation("Unexpected kind for negation")), }, + crate::UnaryOperator::LogicalNot => spirv::Op::LogicalNot, + crate::UnaryOperator::BitwiseNot => spirv::Op::Not, }; block diff --git a/src/back/wgsl/writer.rs b/src/back/wgsl/writer.rs index 36380952af..075d85558c 100644 --- a/src/back/wgsl/writer.rs +++ b/src/back/wgsl/writer.rs @@ -1599,13 +1599,8 @@ impl Writer { Expression::Unary { op, expr } => { let unary = match op { crate::UnaryOperator::Negate => "-", - crate::UnaryOperator::Not => { - match func_ctx.resolve_type(expr, &module.types).scalar_kind() { - Some(crate::ScalarKind::Sint) | Some(crate::ScalarKind::Uint) => "~", - Some(crate::ScalarKind::Bool) => "!", - _ => return Err(Error::Custom("validation failure".to_string())), - } - } + crate::UnaryOperator::LogicalNot => "!", + crate::UnaryOperator::BitwiseNot => "~", }; write!(self.out, "{unary}(")?; diff --git a/src/front/glsl/builtins.rs b/src/front/glsl/builtins.rs index 8d57b66da2..bf7559ee21 100644 --- a/src/front/glsl/builtins.rs +++ b/src/front/glsl/builtins.rs @@ -844,7 +844,7 @@ fn inject_standard_builtins( let fun = match name { "all" => MacroCall::Relational(RelationalFunction::All), "any" => MacroCall::Relational(RelationalFunction::Any), - "not" => MacroCall::Unary(UnaryOperator::Not), + "not" => MacroCall::Unary(UnaryOperator::LogicalNot), _ => unreachable!(), }; diff --git a/src/front/glsl/parser/expressions.rs b/src/front/glsl/parser/expressions.rs index 32e0959c26..1b8febce90 100644 --- a/src/front/glsl/parser/expressions.rs +++ b/src/front/glsl/parser/expressions.rs @@ -296,8 +296,12 @@ impl<'source> ParsingContext<'source> { op: UnaryOperator::Negate, expr, }, - TokenValue::Bang | TokenValue::Tilde => HirExprKind::Unary { - op: UnaryOperator::Not, + TokenValue::Bang => HirExprKind::Unary { + op: UnaryOperator::LogicalNot, + expr, + }, + TokenValue::Tilde => HirExprKind::Unary { + op: UnaryOperator::BitwiseNot, expr, }, _ => return Ok(expr), diff --git a/src/front/glsl/parser/functions.rs b/src/front/glsl/parser/functions.rs index eebaec2698..b63b13a4a3 100644 --- a/src/front/glsl/parser/functions.rs +++ b/src/front/glsl/parser/functions.rs @@ -338,7 +338,7 @@ impl<'source> ParsingContext<'source> { let (expr, expr_meta) = ctx.lower_expect(stmt, frontend, root, ExprPos::Rhs)?; let condition = ctx.add_expression( Expression::Unary { - op: UnaryOperator::Not, + op: UnaryOperator::LogicalNot, expr, }, expr_meta, @@ -393,7 +393,7 @@ impl<'source> ParsingContext<'source> { let (expr, expr_meta) = ctx.lower_expect(stmt, frontend, root, ExprPos::Rhs)?; let condition = ctx.add_expression( Expression::Unary { - op: UnaryOperator::Not, + op: UnaryOperator::LogicalNot, expr, }, expr_meta, @@ -481,7 +481,7 @@ impl<'source> ParsingContext<'source> { let condition = ctx.add_expression( Expression::Unary { - op: UnaryOperator::Not, + op: UnaryOperator::LogicalNot, expr, }, expr_meta, diff --git a/src/front/spv/mod.rs b/src/front/spv/mod.rs index 4bb696a63a..7116925d03 100644 --- a/src/front/spv/mod.rs +++ b/src/front/spv/mod.rs @@ -2559,7 +2559,7 @@ impl> Frontend { &mut block, block_id, body_idx, - crate::UnaryOperator::Not, + crate::UnaryOperator::BitwiseNot, )?; } Op::ShiftRightLogical => { @@ -3033,7 +3033,7 @@ impl> Frontend { // Relational and Logical Instructions Op::LogicalNot => { inst.expect(4)?; - parse_expr_op!(crate::UnaryOperator::Not, UNARY)?; + parse_expr_op!(crate::UnaryOperator::LogicalNot, UNARY)?; } Op::LogicalOr => { inst.expect(5)?; @@ -3249,7 +3249,7 @@ impl> Frontend { } else { ctx.expressions.append( crate::Expression::Unary { - op: crate::UnaryOperator::Not, + op: crate::UnaryOperator::LogicalNot, expr: condition, }, span, diff --git a/src/front/wgsl/parse/mod.rs b/src/front/wgsl/parse/mod.rs index 74738ab231..23398e6116 100644 --- a/src/front/wgsl/parse/mod.rs +++ b/src/front/wgsl/parse/mod.rs @@ -741,11 +741,21 @@ impl Parser { let span = self.peek_rule_span(lexer); ctx.expressions.append(expr, span) } - Token::Operation('!' | '~') => { + Token::Operation('!') => { let _ = lexer.next(); let expr = self.unary_expression(lexer, ctx.reborrow())?; let expr = ast::Expression::Unary { - op: crate::UnaryOperator::Not, + op: crate::UnaryOperator::LogicalNot, + expr, + }; + let span = self.peek_rule_span(lexer); + ctx.expressions.append(expr, span) + } + Token::Operation('~') => { + let _ = lexer.next(); + let expr = self.unary_expression(lexer, ctx.reborrow())?; + let expr = ast::Expression::Unary { + op: crate::UnaryOperator::BitwiseNot, expr, }; let span = self.peek_rule_span(lexer); diff --git a/src/lib.rs b/src/lib.rs index 9d70190421..9b563b94af 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -995,7 +995,8 @@ pub struct LocalVariable { #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] pub enum UnaryOperator { Negate, - Not, + LogicalNot, + BitwiseNot, } /// Operation that can be applied on two values. diff --git a/src/proc/constant_evaluator.rs b/src/proc/constant_evaluator.rs index 70a46114a9..d2604ae1c7 100644 --- a/src/proc/constant_evaluator.rs +++ b/src/proc/constant_evaluator.rs @@ -1019,10 +1019,13 @@ impl<'a> ConstantEvaluator<'a> { Literal::F32(v) => Literal::F32(-v), _ => return Err(ConstantEvaluatorError::InvalidUnaryOpArg), }, - UnaryOperator::Not => match value { + UnaryOperator::LogicalNot => match value { + Literal::Bool(v) => Literal::Bool(!v), + _ => return Err(ConstantEvaluatorError::InvalidUnaryOpArg), + }, + UnaryOperator::BitwiseNot => match value { Literal::I32(v) => Literal::I32(!v), Literal::U32(v) => Literal::U32(!v), - Literal::Bool(v) => Literal::Bool(!v), _ => return Err(ConstantEvaluatorError::InvalidUnaryOpArg), }, }), @@ -1344,12 +1347,12 @@ mod tests { }; let expr3 = Expression::Unary { - op: UnaryOperator::Not, + op: UnaryOperator::BitwiseNot, expr, }; let expr4 = Expression::Unary { - op: UnaryOperator::Not, + op: UnaryOperator::BitwiseNot, expr: expr1, }; diff --git a/src/valid/expression.rs b/src/valid/expression.rs index 95225a3926..cc77de38b0 100644 --- a/src/valid/expression.rs +++ b/src/valid/expression.rs @@ -649,10 +649,9 @@ impl super::Validator { use crate::UnaryOperator as Uo; let inner = &resolver[expr]; match (op, inner.scalar_kind()) { - (_, Some(Sk::Sint | Sk::Bool)) - //TODO: restrict Negate for bools? - | (Uo::Negate, Some(Sk::Float)) - | (Uo::Not, Some(Sk::Uint)) => {} + (Uo::Negate, Some(Sk::Float | Sk::Sint)) + | (Uo::LogicalNot, Some(Sk::Bool)) + | (Uo::BitwiseNot, Some(Sk::Sint | Sk::Uint)) => {} other => { log::error!("Op {:?} kind {:?}", op, other); return Err(ExpressionError::InvalidUnaryOperandType(op, expr));