Skip to content
This repository has been archived by the owner on Jan 29, 2025. It is now read-only.

[glsl-new] Handle local vars scopes #126

Merged
merged 2 commits into from
Aug 13, 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
45 changes: 43 additions & 2 deletions src/front/glsl_new/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ impl Program {
context: Context {
expressions: Arena::<Expression>::new(),
local_variables: Arena::<LocalVariable>::new(),
lookup_local_variables: FastHashMap::default(),
scopes: vec![FastHashMap::default()],
},
}
}
Expand All @@ -49,7 +49,48 @@ pub enum Profile {
pub struct Context {
pub expressions: Arena<Expression>,
pub local_variables: Arena<LocalVariable>,
pub lookup_local_variables: FastHashMap<String, Handle<LocalVariable>>,
//TODO: Find less allocation heavy representation
pub scopes: Vec<FastHashMap<String, Handle<LocalVariable>>>,
}

impl Context {
pub fn lookup_local_var(&self, name: &str) -> Option<Handle<LocalVariable>> {
for scope in self.scopes.iter().rev() {
if let Some(var) = scope.get(name) {
return Some(*var);
}
}
None
}

pub fn lookup_local_var_current_scope(&self, name: &str) -> Option<Handle<LocalVariable>> {
if let Some(current) = self.scopes.last() {
current.get(name).cloned()
} else {
None
}
}

pub fn clear_scopes(&mut self) {
self.scopes.clear();
self.scopes.push(FastHashMap::default());
}

/// Add variable to current scope
pub fn add_local_var(&mut self, name: String, handle: Handle<LocalVariable>) {
if let Some(current) = self.scopes.last_mut() {
(*current).insert(name, handle);
}
}

/// Add new empty scope
pub fn push_scope(&mut self) {
self.scopes.push(FastHashMap::default());
}

pub fn remove_current_scope(&mut self) {
self.scopes.pop();
}
}

#[derive(Debug)]
Expand Down
4 changes: 4 additions & 0 deletions src/front/glsl_new/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub enum ErrorKind {
ParserStackOverflow,
NotImplemented(&'static str),
UnknownVariable(TokenMetadata, String),
VariableAlreadyDeclared(String),
ExpectedConstant,
SemanticError(&'static str),
}
Expand All @@ -37,6 +38,9 @@ impl fmt::Display for ErrorKind {
ErrorKind::UnknownVariable(meta, val) => {
write!(f, "Unknown variable {} at {:?}", val, meta)
}
ErrorKind::VariableAlreadyDeclared(val) => {
Copy link
Member

Choose a reason for hiding this comment

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

who forbids this? I think, if you encounter a variable with the same name, it can just replace the current one in the scope map

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will try checking if the spec has anything to say on this ...

Copy link
Member

Choose a reason for hiding this comment

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

Naga IR doesn't really care about names, other than for entry points (since they connect to the API). So IR has no limitations on what the names are, might as well treat them as debug labels.
The limitation you are talking about comes from GLSL, and the front-end should assume it has valid GLSL on the input, you don't need to go into the forest of validating incoming GLSL sources :) So you don't need to worry about overlaps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Where would the GLSL source be validated? Is naga always supposed to be used with a validator in front of it?

Copy link
Member

Choose a reason for hiding this comment

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

Naga has one and only place in its pipeline where the data is guaranteed to be valid: that's the IR:

  1. front-end inputs are assumed to be valid. We don't care if they actually are, as long as the front-end doesn't crash or hangs (which is a universal requirement to the code base).
  2. front-end output IR has to be valid, and we are testing this, and any clients will be testing this, using the validator in of Validation function for the IR #59 . Any breaking of this contract is a bug in the front-end.
  3. similarly, backends can assume the IR is valid
  4. similarly, backend's produced low-level shaders have to be valid, and we are either testing, or will be testing this

So Naga doesn't care if your GLSL is valid or not. Just like a Vulkan driver doesn't care if the use of the API is valid or not. Unlike the Vulkan driver, Naga can't afford to behave strangely though.

write!(f, "Variable {} already decalred in current scope", val)
}
ErrorKind::ExpectedConstant => write!(f, "Expected constant"),
ErrorKind::SemanticError(msg) => write!(f, "Semantic error: {}", msg),
}
Expand Down
41 changes: 27 additions & 14 deletions src/front/glsl_new/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,18 +156,17 @@ pomelo! {
}
} else {
// try global and local vars
if let Some(global_var) = extra.lookup_global_variables.get(&v.1) {
ExpressionRule{
expression: extra.context.expressions.append(Expression::GlobalVariable(*global_var)),
statements: vec![],
}
} else if let Some(local_var) = extra.context.lookup_local_variables.get(&v.1) {
ExpressionRule{
expression: extra.context.expressions.append(Expression::LocalVariable(*local_var)),
statements: vec![],
}
} else {
return Err(ErrorKind::UnknownVariable(v.0, v.1))
let expr =
if let Some(local_var) = extra.context.lookup_local_var(&v.1) {
Expression::LocalVariable(local_var)
} else if let Some(global_var) = extra.lookup_global_variables.get(&v.1) {
Expression::GlobalVariable(*global_var)
} else {
return Err(ErrorKind::UnknownVariable(v.0, v.1))
};
ExpressionRule{
expression: extra.context.expressions.append(expr),
statements: vec![],
}
}
}
Expand Down Expand Up @@ -545,6 +544,10 @@ pomelo! {
let mut statements = Vec::<Statement>::new();
// local variables
for (id, initializer) in d.ids_initializers {
// check if already declared in current scope
if extra.context.lookup_local_var_current_scope(&id).is_some() {
return Err(ErrorKind::VariableAlreadyDeclared(id))
}
let h = extra.context.local_variables.append(
LocalVariable {
name: Some(id.clone()),
Expand All @@ -555,7 +558,7 @@ pomelo! {
}),
}
);
extra.context.lookup_local_variables.insert(id, h);
extra.context.add_local_var(id, h);
}
match statements.len() {
1 => statements.remove(0),
Expand All @@ -577,7 +580,16 @@ pomelo! {
//simple_statement ::= jump_statement;

compound_statement ::= LeftBrace RightBrace {vec![]}
compound_statement ::= LeftBrace statement_list(sl) RightBrace {sl}
compound_statement ::= left_brace_scope statement_list(sl) RightBrace {
extra.context.remove_current_scope();
sl
}

// extra rule to add scope before statement_list
left_brace_scope ::= LeftBrace {
extra.context.push_scope();
}


compound_statement_no_new_scope ::= LeftBrace RightBrace {vec![]}
compound_statement_no_new_scope ::= LeftBrace statement_list(sl) RightBrace {sl}
Expand Down Expand Up @@ -700,6 +712,7 @@ pomelo! {
function_definition ::= function_prototype(mut f) compound_statement_no_new_scope(cs) {
std::mem::swap(&mut f.expressions, &mut extra.context.expressions);
std::mem::swap(&mut f.local_variables, &mut extra.context.local_variables);
extra.context.clear_scopes();
f.body = cs;
f
};
Expand Down