Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

glsl-in: Fix the ternary operator to behave like an if #1877

Merged
merged 3 commits into from
May 1, 2022
Merged

glsl-in: Fix the ternary operator to behave like an if #1877

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4acc1b9
glsl-in: Fix the ternary to behave as an if
JCapucho Apr 30, 2022
66c6570
glsl-in: Improve the context emitter interface
JCapucho Apr 30, 2022
49fc0ae
glsl-in: Apply comments
JCapucho May 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/front/glsl/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,17 @@ pub enum HirExprKind {
},
Variable(VariableReference),
Call(FunctionCall),
/// Represents the ternary operator in glsl (`:?`)
Conditional {
/// The expression that will decide which branch to take, must evaluate to a boolean
condition: Handle<HirExpr>,
/// The expression that will be evaluated if [`condition`][condition] returns `true`
///
/// [condition]: Self::Conditional::condition
accept: Handle<HirExpr>,
/// The expression that will be evaluated if [`condition`][condition] returns `false`
///
/// [condition]: Self::Conditional::condition
reject: Handle<HirExpr>,
},
Assign {
Expand Down
6 changes: 2 additions & 4 deletions src/front/glsl/builtins.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1837,7 +1837,7 @@ impl MacroCall {
MacroCall::ImageStore => {
let comps =
parser.coordinate_components(ctx, args[0], args[1], None, meta, body)?;
ctx.emit_flush(body);
ctx.emit_restart(body);
body.push(
crate::Statement::ImageStore {
image: args[0],
Expand All @@ -1847,7 +1847,6 @@ impl MacroCall {
},
meta,
);
ctx.emit_start();
return Ok(None);
}
MacroCall::MathFunction(fun) => ctx.add_expression(
Expand Down Expand Up @@ -2038,8 +2037,7 @@ impl MacroCall {
body,
),
MacroCall::Barrier => {
ctx.emit_flush(body);
ctx.emit_start();
ctx.emit_restart(body);
body.push(crate::Statement::Barrier(crate::Barrier::all()), meta);
return Ok(None);
}
Expand Down
162 changes: 141 additions & 21 deletions src/front/glsl/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ impl Context {
}: GlobalLookup,
body: &mut Block,
) {
self.emit_flush(body);
self.emit_end(body);
let (expr, load, constant) = match kind {
GlobalLookupKind::Variable(v) => {
let span = parser.module.global_variables.get_span(v);
Expand Down Expand Up @@ -170,14 +170,37 @@ impl Context {
self.lookup_global_var_exps.insert(name.into(), var);
}

/// Starts the expression emitter
///
/// # Panics
///
/// - If called twice in a row without calling [`emit_end`][Self::emit_end].
#[inline]
pub fn emit_start(&mut self) {
self.emitter.start(&self.expressions)
}

pub fn emit_flush(&mut self, body: &mut Block) {
/// Emits all the expressions captured by the emitter to the passed `body`
///
/// # Panics
///
/// - If called before calling [`emit_start`][Self::emit_start].
/// - If called twice in a row without calling [`emit_start`][Self::emit_start].
pub fn emit_end(&mut self, body: &mut Block) {
body.extend(self.emitter.finish(&self.expressions))
}

/// Emits all the expressions captured by the emitter to the passed `body`
/// and starts the emitter again
///
/// # Panics
///
/// - If called before calling [`emit_start`][Self::emit_start].
pub fn emit_restart(&mut self, body: &mut Block) {
self.emit_end(body);
self.emit_start()
}

pub fn add_expression(
&mut self,
expr: Expression,
Expand All @@ -186,7 +209,7 @@ impl Context {
) -> Handle<Expression> {
let needs_pre_emit = expr.needs_pre_emit();
if needs_pre_emit {
self.emit_flush(body);
self.emit_end(body);
}
let handle = self.expressions.append(expr, meta);
if needs_pre_emit {
Expand Down Expand Up @@ -292,8 +315,7 @@ impl Context {
);
let local_expr = self.add_expression(Expression::LocalVariable(handle), meta, body);

self.emit_flush(body);
self.emit_start();
self.emit_restart(body);

body.push(
Statement::Store {
Expand Down Expand Up @@ -462,8 +484,7 @@ impl Context {
body,
);

self.emit_flush(body);
self.emit_start();
self.emit_restart(body);

body.push(
Statement::Store {
Expand All @@ -474,8 +495,7 @@ impl Context {
);
}
} else {
self.emit_flush(body);
self.emit_start();
self.emit_restart(body);

body.push(Statement::Store { pointer, value }, meta);
}
Expand Down Expand Up @@ -1069,35 +1089,135 @@ impl Context {
)?;
return Ok((maybe_expr, meta));
}
// `HirExprKind::Conditional` represents the ternary operator in glsl (`:?`)
//
// The ternary operator is defined to only evaluate one of the two possible
// expressions which means that it's behavior is that of an `if` statement,
// and it's merely syntatic sugar for it.
HirExprKind::Conditional {
condition,
accept,
reject,
} if ExprPos::Lhs != pos => {
jimblandy marked this conversation as resolved.
Show resolved Hide resolved
// Lower the condition first to the current bodyy
JCapucho marked this conversation as resolved.
Show resolved Hide resolved
let condition = self
.lower_expect_inner(stmt, parser, condition, ExprPos::Rhs, body)?
.0;

// Emit all expressions since we will be adding statements to
// other bodies next
self.emit_restart(body);

// Create the bodies for the two cases
let mut accept_body = Block::new();
let mut reject_body = Block::new();

// Lower the `true` branch
let (mut accept, accept_meta) =
self.lower_expect_inner(stmt, parser, accept, pos, body)?;
self.lower_expect_inner(stmt, parser, accept, pos, &mut accept_body)?;

// Flush the body of the `true` branch, to start emitting on the
// `false` branch
self.emit_restart(&mut accept_body);

// Lower the `false` branch
let (mut reject, reject_meta) =
self.lower_expect_inner(stmt, parser, reject, pos, body)?;
self.lower_expect_inner(stmt, parser, reject, pos, &mut reject_body)?;

// Flush the body of the `false` branch
self.emit_restart(&mut reject_body);

// We need to do some custom implicit conversions since the two target expressions
// are in different bodies
if let (
Some((accept_power, accept_width, accept_kind)),
Some((reject_power, reject_width, reject_kind)),
) = (
// Get the components of both branches and calculate the type power
self.expr_scalar_components(parser, accept, accept_meta)?
.and_then(|(kind, width)| Some((type_power(kind, width)?, width, kind))),
self.expr_scalar_components(parser, reject, reject_meta)?
.and_then(|(kind, width)| Some((type_power(kind, width)?, width, kind))),
) {
match accept_power.cmp(&reject_power) {
std::cmp::Ordering::Less => {
self.conversion(&mut accept, accept_meta, reject_kind, reject_width)?;
// The expression belongs to the `true` branch so we need to flush to
// the respective body
self.emit_end(&mut accept_body);
}
// Technically there's nothing to flush but we need to add some
JCapucho marked this conversation as resolved.
Show resolved Hide resolved
// expressions that must not be emitted so instead of flushing,
// starting and flushing again, just make sure everything is
// flushed.
std::cmp::Ordering::Equal => self.emit_end(body),
std::cmp::Ordering::Greater => {
self.conversion(&mut reject, reject_meta, accept_kind, accept_width)?;
// The expression belongs to the `false` branch so we need to flush to
// the respective body
self.emit_end(&mut reject_body);
}
}
}

self.binary_implicit_conversion(
parser,
&mut accept,
// We need to get the type of the resulting expression to create the local,
// this must be done after implicit conversions to ensure both branches have
// the same type.
let ty = parser.resolve_type_handle(self, accept, accept_meta)?;

// Add the local that will hold the result of our conditional
let local = self.locals.append(
LocalVariable {
name: None,
ty,
init: None,
},
Span::default(),
JCapucho marked this conversation as resolved.
Show resolved Hide resolved
);

// Note: `Expression::LocalVariable` must not be emited so it's important
// that at this point the emitter is flushed but not started.
let local_expr = self
.expressions
.append(Expression::LocalVariable(local), meta);

// Add to each body the store to the result variable
accept_body.push(
Statement::Store {
pointer: local_expr,
value: accept,
},
accept_meta,
&mut reject,
);
reject_body.push(
Statement::Store {
pointer: local_expr,
value: reject,
},
reject_meta,
)?;
);

self.add_expression(
Expression::Select {
// Finally add the `If` to the main body with the `condition` we lowered
// earlier and the branches we prepared.
body.push(
Statement::If {
JCapucho marked this conversation as resolved.
Show resolved Hide resolved
condition,
accept,
reject,
accept: accept_body,
reject: reject_body,
},
meta,
);

// Restart the emitter
self.emit_start();

// Note: `Expression::Load` must be emited before it's used so make
// sure the emitter is active here.
self.expressions.append(
Expression::Load {
pointer: local_expr,
},
meta,
body,
)
}
HirExprKind::Assign { tgt, value } if ExprPos::Lhs != pos => {
Expand Down
5 changes: 2 additions & 3 deletions src/front/glsl/functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,7 +972,7 @@ impl Parser {

match kind {
FunctionKind::Call(function) => {
ctx.emit_flush(body);
ctx.emit_end(body);

let result = if !is_void {
Some(ctx.add_expression(Expression::CallResult(function), meta, body))
Expand All @@ -995,8 +995,7 @@ impl Parser {
for (original, pointer) in proxy_writes {
let value = ctx.add_expression(Expression::Load { pointer }, meta, body);

ctx.emit_flush(body);
ctx.emit_start();
ctx.emit_restart(body);

body.push(
Statement::Store {
Expand Down
9 changes: 7 additions & 2 deletions src/front/glsl/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,8 +438,13 @@ impl<'ctx, 'qualifiers> DeclarationContext<'ctx, 'qualifiers> {
}
}

/// Emits all the expressions captured by the emitter and starts the emitter again
///
/// Alias to [`emit_restart`][emit_restart] with the declaration body
///
/// [emit_restart]: Context::emit_restart
#[inline]
fn flush_expressions(&mut self) {
self.ctx.emit_flush(self.body);
self.ctx.emit_start()
self.ctx.emit_restart(self.body);
}
}
21 changes: 7 additions & 14 deletions src/front/glsl/parser/functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,7 @@ impl<'source> ParsingContext<'source> {
}
};

ctx.emit_flush(body);
ctx.emit_start();
ctx.emit_restart(body);

body.push(Statement::Return { value }, meta);
terminator.get_or_insert(body.len());
Expand Down Expand Up @@ -132,8 +131,7 @@ impl<'source> ParsingContext<'source> {
};
self.expect(parser, TokenValue::RightParen)?;

ctx.emit_flush(body);
ctx.emit_start();
ctx.emit_restart(body);

let mut accept = Block::new();
if let Some(more_meta) =
Expand Down Expand Up @@ -176,8 +174,7 @@ impl<'source> ParsingContext<'source> {

self.expect(parser, TokenValue::RightParen)?;

ctx.emit_flush(body);
ctx.emit_start();
ctx.emit_restart(body);

let mut cases = Vec::new();

Expand Down Expand Up @@ -301,8 +298,7 @@ impl<'source> ParsingContext<'source> {
&mut loop_body,
);

ctx.emit_flush(&mut loop_body);
ctx.emit_start();
ctx.emit_restart(&mut loop_body);

loop_body.push(
Statement::If {
Expand Down Expand Up @@ -359,8 +355,7 @@ impl<'source> ParsingContext<'source> {
&mut loop_body,
);

ctx.emit_flush(&mut loop_body);
ctx.emit_start();
ctx.emit_restart(&mut loop_body);

loop_body.push(
Statement::If {
Expand Down Expand Up @@ -427,8 +422,7 @@ impl<'source> ParsingContext<'source> {

let pointer = parser.add_local_var(ctx, &mut block, decl)?;

ctx.emit_flush(&mut block);
ctx.emit_start();
ctx.emit_restart(&mut block);

block.push(Statement::Store { pointer, value }, meta);

Expand All @@ -448,8 +442,7 @@ impl<'source> ParsingContext<'source> {
&mut block,
);

ctx.emit_flush(&mut block);
ctx.emit_start();
ctx.emit_restart(&mut block);

block.push(
Statement::If {
Expand Down
Loading