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.

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

GLSL local variables, interface blocks, and XOR #258

Merged
merged 3 commits into from
Nov 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/back/glsl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1790,6 +1790,7 @@ fn collect_texture_mapping<'a>(
for func in functions {
let mut interface = Interface {
expressions: &func.expressions,
local_variables: &func.local_variables,
visitor: TextureMappingVisitor {
expressions: &func.expressions,
map: &mut mappings,
Expand Down
4 changes: 2 additions & 2 deletions src/front/glsl/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ impl Program {
pub fn binary_expr(
&mut self,
op: BinaryOperator,
left: ExpressionRule,
right: ExpressionRule,
left: &ExpressionRule,
right: &ExpressionRule,
) -> ExpressionRule {
ExpressionRule::from_expression(self.context.expressions.append(Expression::Binary {
op,
Expand Down
94 changes: 58 additions & 36 deletions src/front/glsl/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ pomelo! {
use crate::{proc::Typifier, Arena, BinaryOperator, Binding, Block, Constant,
ConstantInner, EntryPoint, Expression, FallThrough, FastHashMap, Function, GlobalVariable, Handle, Interpolation,
LocalVariable, MemberOrigin, SampleLevel, ScalarKind, Statement, StorageAccess,
StorageClass, StructMember, Type, TypeInner};
StorageClass, StructMember, Type, TypeInner, UnaryOperator};
}
%token #[derive(Debug)] #[cfg_attr(test, derive(PartialEq))] pub enum Token {};
%parser pub struct Parser<'a> {};
Expand Down Expand Up @@ -150,9 +150,7 @@ pomelo! {
let var = extra.lookup_variable(&v.1)?;
match var {
Some(expression) => {
ExpressionRule::from_expression(
expression
)
ExpressionRule::from_expression(expression)
},
None => {
return Err(ErrorKind::UnknownVariable(v.0, v.1));
Expand Down Expand Up @@ -246,11 +244,11 @@ pomelo! {
function_call ::= function_call_or_method(fc) {
match fc.kind {
FunctionCallKind::TypeConstructor(ty) => {
let h = extra.context.expressions.append(Expression::Compose{
let h = extra.context.expressions.append(Expression::Compose {
ty,
components: fc.args.iter().map(|a| a.expression).collect(),
});
ExpressionRule{
ExpressionRule {
expression: h,
statements: fc.args.into_iter().map(|a| a.statements).flatten().collect(),
sampler: None
Expand All @@ -270,7 +268,7 @@ pomelo! {
//TODO: check args len
if let Some(sampler) = fc.args[0].sampler {
ExpressionRule{
expression: extra.context.expressions.append(Expression::ImageSample{
expression: extra.context.expressions.append(Expression::ImageSample {
image: fc.args[0].expression,
sampler,
coordinate: fc.args[1].expression,
Expand Down Expand Up @@ -358,73 +356,76 @@ pomelo! {
unary_operator ::= Tilde;
multiplicative_expression ::= unary_expression;
multiplicative_expression ::= multiplicative_expression(left) Star unary_expression(right) {
extra.binary_expr(BinaryOperator::Multiply, left, right)
extra.binary_expr(BinaryOperator::Multiply, &left, &right)
}
multiplicative_expression ::= multiplicative_expression(left) Slash unary_expression(right) {
extra.binary_expr(BinaryOperator::Divide, left, right)
extra.binary_expr(BinaryOperator::Divide, &left, &right)
}
multiplicative_expression ::= multiplicative_expression(left) Percent unary_expression(right) {
extra.binary_expr(BinaryOperator::Modulo, left, right)
extra.binary_expr(BinaryOperator::Modulo, &left, &right)
}
additive_expression ::= multiplicative_expression;
additive_expression ::= additive_expression(left) Plus multiplicative_expression(right) {
extra.binary_expr(BinaryOperator::Add, left, right)
extra.binary_expr(BinaryOperator::Add, &left, &right)
}
additive_expression ::= additive_expression(left) Dash multiplicative_expression(right) {
extra.binary_expr(BinaryOperator::Subtract, left, right)
extra.binary_expr(BinaryOperator::Subtract, &left, &right)
}
shift_expression ::= additive_expression;
shift_expression ::= shift_expression(left) LeftOp additive_expression(right) {
extra.binary_expr(BinaryOperator::ShiftLeft, left, right)
extra.binary_expr(BinaryOperator::ShiftLeft, &left, &right)
}
shift_expression ::= shift_expression(left) RightOp additive_expression(right) {
extra.binary_expr(BinaryOperator::ShiftRight, left, right)
extra.binary_expr(BinaryOperator::ShiftRight, &left, &right)
}
relational_expression ::= shift_expression;
relational_expression ::= relational_expression(left) LeftAngle shift_expression(right) {
extra.binary_expr(BinaryOperator::Less, left, right)
extra.binary_expr(BinaryOperator::Less, &left, &right)
}
relational_expression ::= relational_expression(left) RightAngle shift_expression(right) {
extra.binary_expr(BinaryOperator::Greater, left, right)
extra.binary_expr(BinaryOperator::Greater, &left, &right)
}
relational_expression ::= relational_expression(left) LeOp shift_expression(right) {
extra.binary_expr(BinaryOperator::LessEqual, left, right)
extra.binary_expr(BinaryOperator::LessEqual, &left, &right)
}
relational_expression ::= relational_expression(left) GeOp shift_expression(right) {
extra.binary_expr(BinaryOperator::GreaterEqual, left, right)
extra.binary_expr(BinaryOperator::GreaterEqual, &left, &right)
}
equality_expression ::= relational_expression;
equality_expression ::= equality_expression(left) EqOp relational_expression(right) {
extra.binary_expr(BinaryOperator::Equal, left, right)
extra.binary_expr(BinaryOperator::Equal, &left, &right)
}
equality_expression ::= equality_expression(left) NeOp relational_expression(right) {
extra.binary_expr(BinaryOperator::NotEqual, left, right)
extra.binary_expr(BinaryOperator::NotEqual, &left, &right)
}
and_expression ::= equality_expression;
and_expression ::= and_expression(left) Ampersand equality_expression(right) {
extra.binary_expr(BinaryOperator::And, left, right)
extra.binary_expr(BinaryOperator::And, &left, &right)
}
exclusive_or_expression ::= and_expression;
exclusive_or_expression ::= exclusive_or_expression(left) Caret and_expression(right) {
extra.binary_expr(BinaryOperator::ExclusiveOr, left, right)
extra.binary_expr(BinaryOperator::ExclusiveOr, &left, &right)
}
inclusive_or_expression ::= exclusive_or_expression;
inclusive_or_expression ::= inclusive_or_expression(left) VerticalBar exclusive_or_expression(right) {
extra.binary_expr(BinaryOperator::InclusiveOr, left, right)
extra.binary_expr(BinaryOperator::InclusiveOr, &left, &right)
}
logical_and_expression ::= inclusive_or_expression;
logical_and_expression ::= logical_and_expression(left) AndOp inclusive_or_expression(right) {
extra.binary_expr(BinaryOperator::LogicalAnd, left, right)
extra.binary_expr(BinaryOperator::LogicalAnd, &left, &right)
}
logical_xor_expression ::= logical_and_expression;
logical_xor_expression ::= logical_xor_expression(left) XorOp logical_and_expression(right) {
return Err(ErrorKind::NotImplemented("logical xor"))
//TODO: naga doesn't have BinaryOperator::LogicalXor
// extra.context.expressions.append(Expression::Binary{op: BinaryOperator::LogicalXor, left, right})
let exp1 = extra.binary_expr(BinaryOperator::LogicalOr, &left, &right);
let exp2 = {
let tmp = extra.binary_expr(BinaryOperator::LogicalAnd, &left, &right).expression;
ExpressionRule::from_expression(extra.context.expressions.append(Expression::Unary { op: UnaryOperator::Not, expr: tmp }))
};
extra.binary_expr(BinaryOperator::LogicalAnd, &exp1, &exp2)
}
logical_or_expression ::= logical_xor_expression;
logical_or_expression ::= logical_or_expression(left) OrOp logical_xor_expression(right) {
extra.binary_expr(BinaryOperator::LogicalOr, left, right)
extra.binary_expr(BinaryOperator::LogicalOr, &left, &right)
}

conditional_expression ::= logical_or_expression;
Expand Down Expand Up @@ -498,7 +499,7 @@ pomelo! {
expression ::= assignment_expression;
expression ::= expression(e) Comma assignment_expression(mut ae) {
ae.statements.extend(e.statements);
ExpressionRule{
ExpressionRule {
expression: e.expression,
statements: ae.statements,
sampler: None,
Expand All @@ -521,7 +522,7 @@ pomelo! {

declaration ::= type_qualifier(t) Identifier(i) LeftBrace
struct_declaration_list(sdl) RightBrace Semicolon {
VarDeclaration{
VarDeclaration {
type_qualifiers: t,
ids_initializers: vec![(None, None)],
ty: extra.module.types.fetch_or_append(Type{
Expand All @@ -535,7 +536,7 @@ pomelo! {

declaration ::= type_qualifier(t) Identifier(i1) LeftBrace
struct_declaration_list(sdl) RightBrace Identifier(i2) Semicolon {
VarDeclaration{
VarDeclaration {
type_qualifiers: t,
ids_initializers: vec![(Some(i2.1), None)],
ty: extra.module.types.fetch_or_append(Type{
Expand Down Expand Up @@ -565,7 +566,7 @@ pomelo! {
single_declaration ::= fully_specified_type(t) {
let ty = t.1.ok_or(ErrorKind::SemanticError("Empty type for declaration"))?;

VarDeclaration{
VarDeclaration {
type_qualifiers: t.0,
ids_initializers: vec![],
ty,
Expand All @@ -574,7 +575,7 @@ pomelo! {
single_declaration ::= fully_specified_type(t) Identifier(i) {
let ty = t.1.ok_or(ErrorKind::SemanticError("Empty type for declaration"))?;

VarDeclaration{
VarDeclaration {
type_qualifiers: t.0,
ids_initializers: vec![(Some(i.1), None)],
ty,
Expand All @@ -585,7 +586,7 @@ pomelo! {
single_declaration ::= fully_specified_type(t) Identifier(i) Equal initializer(init) {
let ty = t.1.ok_or(ErrorKind::SemanticError("Empty type for declaration"))?;

VarDeclaration{
VarDeclaration {
type_qualifiers: t.0,
ids_initializers: vec![(Some(i.1), Some(init))],
ty,
Expand Down Expand Up @@ -715,7 +716,7 @@ pomelo! {

struct_declaration ::= type_specifier(t) struct_declarator_list(sdl) Semicolon {
if let Some(ty) = t {
sdl.iter().map(|name| StructMember{
sdl.iter().map(|name| StructMember {
name: Some(name.clone()),
origin: MemberOrigin::Empty,
ty,
Expand Down Expand Up @@ -1007,7 +1008,7 @@ pomelo! {
Statement::Break
}
jump_statement ::= Return Semicolon {
Statement::Return{ value: None }
Statement::Return { value: None }
}
jump_statement ::= Return expression(mut e) Semicolon {
let ret = Statement::Return{ value: Some(e.expression) };
Expand Down Expand Up @@ -1071,6 +1072,27 @@ pomelo! {
);
if let Some(id) = id {
extra.lookup_global_variables.insert(id, h);
} else {
// variables in interface blocks without an instance name are in the global namespace
// https://www.khronos.org/opengl/wiki/Interface_Block_(GLSL)
if let TypeInner::Struct { members } = &extra.module.types[d.ty].inner {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not match on a reference, instead we should just do ref members. This is more explicit and less sugary.

for m in members {
if let Some(name) = &m.name {
let h = extra
.module
.global_variables
.fetch_or_append(GlobalVariable {
name: Some(name.into()),
class,
binding: binding.clone(),
ty: m.ty,
interpolation,
storage_access: StorageAccess::empty(), // TODO
});
extra.lookup_global_variables.insert(name.into(), h);
}
}
Comment on lines +1079 to +1094
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see how this works - how does it relate to the code here: https://github.com/kristoff3r/naga/blob/c436606d09188efab0057db469802ea01df3230f/src/front/glsl/parser.rs#L971.

Also won't this register the members as 'plain' global vars? What happens when these are referenced - then I don't think it will translate into AccessIndex. (OpAccessChain in spv).

I made a small sample where you can see the spv output for named vs. anon interface block: http://shader-playground.timjones.io/12dc04ae724cbb6aa1ddcf812e638302

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't notice that there was some handling of this already. I'll take a look at it and see if it needs changing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, we'll sort it out :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I shall wait for @pjoe before merging the GLSL-in fixes

}
}
}
}
Expand Down
10 changes: 9 additions & 1 deletion src/proc/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::arena::{Arena, Handle};

pub struct Interface<'a, T> {
pub expressions: &'a Arena<crate::Expression>,
pub local_variables: &'a Arena<crate::LocalVariable>,
pub visitor: T,
}

Expand Down Expand Up @@ -36,7 +37,13 @@ where
self.traverse_expr(comp);
}
}
E::FunctionParameter(_) | E::GlobalVariable(_) | E::LocalVariable(_) => {}
E::FunctionParameter(_) | E::GlobalVariable(_) => {}
E::LocalVariable(var) => {
let var = &self.local_variables[var];
if let Some(init) = var.init {
self.traverse_expr(init);
}
}
E::Load { pointer } => {
self.traverse_expr(pointer);
}
Expand Down Expand Up @@ -201,6 +208,7 @@ impl crate::Function {

let mut io = Interface {
expressions: &self.expressions,
local_variables: &self.local_variables,
visitor: GlobalUseVisitor(&mut self.global_usage),
};
io.traverse(&self.body);
Expand Down