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

src/front/glsl/ast.rs

@@ -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`] returns `true`
+        ///
+        /// [`condition`]: Self::Conditional::condition
         accept: Handle<HirExpr>,
+        /// The expression that will be evaluated if [`condition`] returns `false`
+        ///
+        /// [`condition`]: Self::Conditional::condition
         reject: Handle<HirExpr>,
     },
     Assign {

src/front/glsl/builtins.rs

@@ -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],
@@ -1847,7 +1847,6 @@ impl MacroCall {
                     },
                     meta,
                 );
-                ctx.emit_start();
                 return Ok(None);
             }
             MacroCall::MathFunction(fun) => ctx.add_expression(
@@ -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);
             }

src/front/glsl/context.rs

@@ -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);
@@ -170,14 +170,39 @@ 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`].
+    /// - If called twice in a row without calling [`emit_start`].
+    ///
+    /// [`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,
@@ -186,7 +211,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 {
@@ -292,8 +317,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 {
@@ -462,8 +486,7 @@ impl Context {
                     body,
                 );
 
-                self.emit_flush(body);
-                self.emit_start();
+                self.emit_restart(body);
 
                 body.push(
                     Statement::Store {
@@ -474,8 +497,7 @@ impl Context {
                 );
             }
         } else {
-            self.emit_flush(body);
-            self.emit_start();
+            self.emit_restart(body);
 
             body.push(Statement::Store { pointer, value }, meta);
         }
@@ -1069,35 +1091,149 @@ 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 => {
+                // Given an expression `a ? b : c`, we need to produce a Naga
+                // statement roughly like:
+                //
+                //     var temp;
+                //     if a {
+                //         temp = convert(b);
+                //     } else  {
+                //         temp = convert(c);
+                //     }
+                //
+                // where `convert` stands for type conversions to bring `b` and `c` to
+                // the same type, and then use `temp` to represent the value of the whole
+                // conditional expression in subsequent code.
+
+                // Lower the condition first to the current bodyy
                 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 later we will need to
+                        // add some 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,
+                    },
+                    meta,
+                );
+
+                // 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 {
                         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 => {

src/front/glsl/functions.rs

@@ -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))
@@ -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 {

src/front/glsl/parser.rs

@@ -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`] 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);
     }
 }