Skip to content

Commit

Permalink
Split UnaryOperator::Not into UnaryOperator::LogicalNot & `UnaryO…
Browse files Browse the repository at this point in the history
…perator::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)
  • Loading branch information
teoxoy authored and jimblandy committed Oct 16, 2023
1 parent 841d360 commit 04562de
Show file tree
Hide file tree
Showing 13 changed files with 58 additions and 88 deletions.
40 changes: 10 additions & 30 deletions src/back/glsl/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;

Expand Down
18 changes: 3 additions & 15 deletions src/back/hlsl/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand Down
10 changes: 3 additions & 7 deletions src/back/msl/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1551,14 +1551,10 @@ impl<W: Write> Writer<W> {
}
},
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)?;
Expand Down
12 changes: 3 additions & 9 deletions src/back/spv/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 2 additions & 7 deletions src/back/wgsl/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1599,13 +1599,8 @@ impl<W: Write> Writer<W> {
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}(")?;
Expand Down
2 changes: 1 addition & 1 deletion src/front/glsl/builtins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(),
};

Expand Down
8 changes: 6 additions & 2 deletions src/front/glsl/parser/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
6 changes: 3 additions & 3 deletions src/front/glsl/parser/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -481,7 +481,7 @@ impl<'source> ParsingContext<'source> {

let condition = ctx.add_expression(
Expression::Unary {
op: UnaryOperator::Not,
op: UnaryOperator::LogicalNot,
expr,
},
expr_meta,
Expand Down
6 changes: 3 additions & 3 deletions src/front/spv/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2559,7 +2559,7 @@ impl<I: Iterator<Item = u32>> Frontend<I> {
&mut block,
block_id,
body_idx,
crate::UnaryOperator::Not,
crate::UnaryOperator::BitwiseNot,
)?;
}
Op::ShiftRightLogical => {
Expand Down Expand Up @@ -3033,7 +3033,7 @@ impl<I: Iterator<Item = u32>> Frontend<I> {
// 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)?;
Expand Down Expand Up @@ -3249,7 +3249,7 @@ impl<I: Iterator<Item = u32>> Frontend<I> {
} else {
ctx.expressions.append(
crate::Expression::Unary {
op: crate::UnaryOperator::Not,
op: crate::UnaryOperator::LogicalNot,
expr: condition,
},
span,
Expand Down
14 changes: 12 additions & 2 deletions src/front/wgsl/parse/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
11 changes: 7 additions & 4 deletions src/proc/constant_evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
},
}),
Expand Down Expand Up @@ -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,
};

Expand Down
7 changes: 3 additions & 4 deletions src/valid/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down

0 comments on commit 04562de

Please sign in to comment.