-
Notifications
You must be signed in to change notification settings - Fork 194
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
Spirv lookup refactor #88
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an autogenerated code review, new suggestions: 5
6890e70
to
42f7979
Compare
src/back/spv/writer.rs
Outdated
instruction = self.instruction_type_vector(id, scalar_id, size); | ||
self.lookup_type.insert(id, handle); | ||
self.lookup_type.insert(id, lookup_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still gives a compile error, because it expects a value type instead of the reference type. Problem is if I dereference it, I get the error: "Cannot move".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like you are doing this self.lookup_type.insert(id, lookup_type);
pretty much uniformly in this function.
Could it be moved outside of match
?
And then, since this function needs it by value, we likely need to change the parameter to be passed by value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't work because you don't have ownership over lookup_type, only a shared reference. Handles implement the Copy trait which is why it worked before, because it automatically copied the value.
While I haven't looked much at the code it looks like you actually do need to copy the lookup type here, but that would require crate::Type
to be Clone and that you write LookupType<T: Clone>
everywhere, which is messy. All the problems stem from the LocalType::Local(T)
part, so I would look at another way to represent that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider Local(&'a T)
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried using a lifetime yet, I am still exploring the options. But I indeed stumbled across having to implement Clone for all generics that will be using this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to avoid IR changes for this an see if &'a T
would be sufficient
src/back/spv/writer.rs
Outdated
let ty = match lookup_type { | ||
LookupType::Handle(handle) => &arena[*handle], | ||
LookupType::Local(value) => &value, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels pretty wrong to do it like this. Would it be better to let both LookupTypes implement a trait, where I can retrieve type T from, instead of matching and trying to get the Type (in this case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's fine if we don't do this many times :)
src/back/spv/writer.rs
Outdated
@@ -724,9 +769,13 @@ impl Writer { | |||
fn parse_type_declaration( | |||
&mut self, | |||
arena: &crate::Arena<crate::Type>, | |||
handle: crate::Handle<crate::Type>, | |||
lookup_type: &LookupType<crate::Type>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change this to a value type, and deference it at the inserts in this function. Problem is that in get_type_id
I get the "Cannot move" error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please tell more!
src/back/spv/writer.rs
Outdated
Local(T), | ||
} | ||
|
||
impl<T> PartialEq for LookupType<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure derive(PartialEq)
would give you the same implementation. Any reason we do it manually here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because when I don't implement this, I get the error: "Binary operations '==' cannot be applied to type '&back::spv::LookupType<>'
src/back/spv/writer.rs
Outdated
} | ||
|
||
trait LookupHelperNew<T> { | ||
fn get_id(&self, value: &LookupType<T>) -> Option<&Word>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should return Word
without a reference
src/back/spv/writer.rs
Outdated
) -> (Instruction, Word) { | ||
let ty = &arena[handle]; | ||
let ty = match lookup_type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't match on a reference, instead let's match the type match *lookup_type
src/back/spv/writer.rs
Outdated
let ty = match lookup_type { | ||
LookupType::Handle(handle) => &arena[*handle], | ||
LookupType::Local(value) => &value, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's fine if we don't do this many times :)
src/back/spv/writer.rs
Outdated
instruction = self.instruction_type_vector(id, scalar_id, size); | ||
self.lookup_type.insert(id, handle); | ||
self.lookup_type.insert(id, lookup_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like you are doing this self.lookup_type.insert(id, lookup_type);
pretty much uniformly in this function.
Could it be moved outside of match
?
And then, since this function needs it by value, we likely need to change the parameter to be passed by value
src/back/spv/writer.rs
Outdated
@@ -724,9 +769,13 @@ impl Writer { | |||
fn parse_type_declaration( | |||
&mut self, | |||
arena: &crate::Arena<crate::Type>, | |||
handle: crate::Handle<crate::Type>, | |||
lookup_type: &LookupType<crate::Type>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please tell more!
f9310ef
to
e80f414
Compare
e80f414
to
05da859
Compare
src/back/spv/writer.rs
Outdated
let new_ty: &'a crate::Type = &crate::Type { | ||
name: None, | ||
inner: crate::TypeInner::Scalar { | ||
width, | ||
kind | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are constructing the type on the stack, and then getting a reference to it. This doesn't fulfill "`a".
I think your current LookupType::Local
variant is dysfunctional: the only thing in "'a" that owns the types is the module's arenat, and if you have the type there, you might as well use LookupType::Handle
.
src/back/spv/writer.rs
Outdated
{ | ||
fn get_id(&self, value: &LookupType<'a, T>) -> Option<&Word> { | ||
let mut result = None; | ||
for (k, v) in self.iter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be expressed a bit nicer, something like self.iter().find(|&(k, v)| v == value).map(|(k, _)| k)
src/back/spv/writer.rs
Outdated
let new_ty: &'a crate::Type = &crate::Type { | ||
name: None, | ||
inner: crate::TypeInner::Scalar { | ||
width, | ||
kind | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are constructing the type on the stack, and then getting a reference to it. This doesn't fulfill "`a".
I think your current LookupType::Local
variant is dysfunctional: the only thing in "'a" that owns the types is the module's arenat, and if you have the type there, you might as well use LookupType::Handle
.
26c97b5
to
4acb5cf
Compare
src/back/spv/writer.rs
Outdated
LookupType::Local(local_ty) => { | ||
match local_ty { | ||
LocalType::Scalar(ty_inner) => &ty.inner == ty_inner, | ||
// TODO Pointers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had an equivalent check for LocalType::Pointer
, but it caused a weird behavior that a OpTypePointer
referencing a result type of OpTypePointer
would be created. Which is not allowed in SPIR-V.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are on the right track!
src/back/spv/writer.rs
Outdated
@@ -66,7 +78,7 @@ pub struct Writer { | |||
annotations: Vec<Instruction>, | |||
writer_flags: WriterFlags, | |||
void_type: Option<u32>, | |||
lookup_type: FastHashMap<Word, crate::Handle<crate::Type>>, | |||
lookup_type_new: FastHashMap<Word, LookupType>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe all of the lookup tables here need to be reversed (were they copied from the front-end?). I.e. you should never need to lookup IR stuff by a spirv ID. You need the opposite: given IR handles and stuff, be able to produce spirv IDs that you are saving to the file. So this table (and all others) need to look like this:
lookup_type: FastHashMap<LocalType, Word>,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done this, as result, I had to modify the lib.rs to include Hash
and Eq
on a couple of structs.
src/back/spv/writer.rs
Outdated
@@ -10,38 +11,19 @@ enum Signedness { | |||
Signed = 1, | |||
} | |||
|
|||
#[derive(Debug)] | |||
#[derive(Hash, Eq, PartialEq, Debug)] | |||
enum LocalType { | |||
Scalar(crate::TypeInner), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we storing TypeInner
here? I.e. if we are going to store TypeInner
, we wouldn't need LocalType
at all :) The idea was to have these types fully specified, i.e.
enum LocalType {
Scalar { kind, width },
Vector { size, kind, width },
Pointer { base, class },
}
src/lib.rs
Outdated
@@ -181,7 +181,7 @@ pub struct Type { | |||
|
|||
/// Enum with additional information, depending on the kind of type. | |||
// Clone is used only for error reporting and is not intended for end users | |||
#[derive(Clone, Debug, PartialEq)] | |||
#[derive(Clone, Debug, PartialEq, Hash, Eq)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with the changes to LocalType
that I suggested, these derives are no longer going to be needed
src/lib.rs
Outdated
@@ -134,15 +134,15 @@ pub enum MemberOrigin { | |||
|
|||
/// Member of a user-defined structure. | |||
// Clone is used only for error reporting and is not intended for end users | |||
#[derive(Clone, Debug, PartialEq)] | |||
#[derive(Clone, Debug, PartialEq, Hash, Eq)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these shouldn't be needed either
eee5807
to
a67419b
Compare
3ef675c
to
70ac39f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, we are almost there :) thank you for your patience!
src/back/spv/writer.rs
Outdated
@@ -1145,20 +1230,24 @@ impl Writer { | |||
crate::TypeInner::Vector { size, kind, width } => { | |||
vector_id = Some(left_id); | |||
for (k, v) in self.lookup_type.iter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we not just issuing a lookup into the map? we shouldn't need to iterate the arenas anywhere more than once, really
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely agree, this is probably the most dirtiest piece of code in this back-end. I will take a look to fix it, so it won't iterate the arena again. This piece of code will be completely refactored in the future I presume, as it only supports very basic cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh and by the way, as I see now, LocalType
's arent even supported here. I will fix that too.
src/back/spv/writer.rs
Outdated
@@ -1169,20 +1258,24 @@ impl Writer { | |||
crate::TypeInner::Vector { size, kind, width } => { | |||
vector_id = Some(right_id); | |||
for (k, v) in self.lookup_type.iter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
No worries at all, we are working towards the same goal. I will look into it later today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
src/back/spv/writer.rs
Outdated
@@ -865,15 +858,21 @@ impl Writer { | |||
|
|||
let instruction; | |||
|
|||
match ty.inner.clone() { | |||
match ty.inner { | |||
crate::TypeInner::Scalar { kind, width } => { | |||
instruction = self.write_scalar(id, kind, width); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should use get_or_create_scalar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that is not possible, get_or_create_scalar
and get_or_create_vector
only returns a Word/Id, not an instruction.
src/back/spv/writer.rs
Outdated
crate::TypeInner::Scalar { kind, width } => { | ||
instruction = self.write_scalar(id, kind, width); | ||
self.lookup_type.insert(LookupType::Handle(handle), id); | ||
self.lookup_type | ||
.insert(LookupType::Local(LocalType::Scalar { kind, width }), id); | ||
} | ||
crate::TypeInner::Vector { size, kind, width } => { | ||
let scalar_id = self.get_or_create_scalar(arena, kind, width); | ||
instruction = self.instruction_type_vector(id, scalar_id, size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should use get_or_create_vector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for updating this!
I see the main concern in that additions to the lookup maps happen at different stack levels, in different places. It would be nice to coalesce them, and also never drop Entry::Vacant
and just use it right away.
There is a problem you'll face though: if Entry::Vacant
is alive, you can't mutate the lookup map for adding dependant entries. So we should collect them somewhere, i.e. a function would have a parameter of type impl Extend<(LocalType, spirv::Id)>
, and after such function is called, the caller can deal with Entry::Vacant
, then release the hold of the map, and then iterate these extra dependents and push insert to the map.
if let Entry::Occupied(e) = self.lookup_type.entry(lookup_ty) { | ||
*e.get() | ||
} else { | ||
match lookup_ty { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we add a new type, we have an ID. So we should write down it in the map, i.e.
match self.lookup_type(lookup_ty) {
Entry::Occupied(e) => e.get(),
Entry::Vacant(e) => {
let id = ...;
*e.insert(id)
}
}
match self.lookup_constant.entry(handle) { | ||
Entry::Occupied(e) => *e.get(), | ||
_ => { | ||
let (instruction, id) = self.write_constant_type(handle, ir_module); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, we should add it to the map with e.insert(id)
match self.lookup_global_variable.lookup_id(handle) { | ||
Some(word) => word, | ||
None => { | ||
match self.lookup_global_variable.entry(handle) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
})) { | ||
Entry::Occupied(e) => *e.get(), | ||
_ => { | ||
let pointer_id = self.generate_id(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here, we should insert it into the map
LocalType::Pointer { .. } => unimplemented!(), | ||
}; | ||
|
||
self.lookup_type.insert(LookupType::Local(local_ty), id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you did here. Let's cut the loop. Instead of doing match (entry) -> call function -> insert entry
just do match (entry) -> insert entry
as we have the Entry::Vacant
there.
src/back/spv/writer.rs
Outdated
let id = match self.lookup_type.entry(lookup_ty) { | ||
Entry::Occupied(e) => *e.get(), | ||
Entry::Vacant(e) => { | ||
let id = match lookup_ty { | ||
LookupType::Handle(handle) => self.write_type_declaration_arena(arena, handle, &mut sink), | ||
LookupType::Local(local_ty) => self.write_type_declaration_local(arena, local_ty), | ||
}; | ||
*e.insert(id) | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am getting the "cannot borrow *self
as mutable more than once at a time" over here. I understand why, but I am not sure yet how to resolve this yet.
The functions have to be mutable, even if we extract the part let id = self.generate_id()
from these functions and use it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, right. I suppose we need to refactor write_type_declaration_arena
to not work with self
. Or at least, that's one way to address this...
if it's too difficult, we can go back to your original code, which I think was correct, and I'll look at it later to try to improve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I would prefer that. This means, the rest of the comments are also resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure! Are we all set for landing then?
src/back/spv/writer.rs
Outdated
}; | ||
|
||
for (k, v) in sink.iter() { | ||
self.lookup_type.insert(*k, *v); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this works, but it is not clean, as the sink will always be initialized even though a LookupType is local.
31eecf0
to
4d71e1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pushing this through!
4d71e1d
to
e4bf0b0
Compare
Fix for #85.