(first steps of) SR module construction from DR #87
Conversation
kvark
force-pushed
the
sr-module
branch
2 times, most recently
from
2137843
to
6e11bff
Compare
|
Is this meant to be reviewed? Would be nice to address the conflicts if so. :) |
There was a problem hiding this comment.
Woo! It looks good, it wil change a bit when rebased but I don't think any major changes are required. The only thing I'm unsure of is whether this is the right place to put it, we could move this into a seperate module as it is essentially a second parser, then again it's all internal so it doesn't matter too much where we put it. (I also go back on my previous statement, I think it's a bad idea to put it in a From impl as they are generally assumed to be reasonably lightweight)
Lastly, it would probably be good to spread about a few more comments as to which bits still need to be implemented just so that nothing gets forgotten.
rspirv/sr/items.rs
Outdated
| /// All entry point declarations, using OpEntryPoint. | ||
| pub entry_points: Vec<instructions::EntryPoint>, | ||
| /// All execution mode declarations, using OpExecutionMode. | ||
| pub execution_modes: Vec<instructions::ExecutionMode>, |
There was a problem hiding this comment.
Why have you removed this?
There was a problem hiding this comment.
because it's a part of struct EntryPoint
rspirv/sr/items.rs
Outdated
| .as_ref() | ||
| .ok_or(ConversionError::MissingFunction)? | ||
| )?; | ||
| let fty = function_type_map |
There was a problem hiding this comment.
It might be sensible to add a check that the function type matches the type of the parameters function (or at least a TODO in there)
rspirv/sr/items.rs
Outdated
| /// All OpExtension instructions. | ||
| pub extensions: Vec<instructions::Extension>, | ||
| pub extensions: Vec<String>, |
There was a problem hiding this comment.
Why have you changed these? I can see pros and cons to both ways round. Also, if this is changed what should happen to the things in instructions atm?
There was a problem hiding this comment.
I don't think we are getting any benefit from using instructions::Extension instead of String, are we? SR module is supposed to be human-friendly, so reducing unnecessary bloat will make it better.
There was a problem hiding this comment.
Seems sensible, just need to be careful that we keep support of unknown extended instruction sets. (i.e. not GLSL or OpenCL)
|
@aioob could you remind me where we stand on the standalone types topic?
|
|
From my point of view, they just aren't needed (although I don't care between 2&3 that much, 3 is preferable as it's less code); they artificially limit what we can represent with no real benefit. However, I haven't looked at the code for a while and would happily accept 1 if anyone can show me a strong benefit it provides. |
|
@antiagainst this is ready to be reviewed now |
|
@aioob @antiagainst anybody?.. review please |
rspirv/lift/mod.rs
Outdated
| .as_ref() | ||
| .ok_or(ConversionError::MissingFunction)? | ||
| )?; | ||
| /* |
There was a problem hiding this comment.
Nit: can we delete commented-out code? They should be added when really supported. Comments are intended for documentation.
rspirv/lift/mod.rs
Outdated
| } | ||
|
|
||
| Ok(module::Module { | ||
| header: match module.header { |
There was a problem hiding this comment.
I don't think the DR header is useful to be carried over to SR generally. (Only the version is useful there. We can discard the other fields.) If this is to be handled later, it would be nice to have a TODO here.
rspirv/sr/module.rs
Outdated
| sr::constants::Constant, | ||
| sr::instructions, | ||
| sr::ops::{self, Op}, | ||
| sr::storage::*, | ||
| sr::types::Type, | ||
| }; | ||
| pub use crate::dr::ModuleHeader; |
There was a problem hiding this comment.
Again, I don't think we should just carray over the module header here. What's the intended usage of it in SR? The version can just be a field on the module.
rspirv/tests/spirv_blobs.rs
Outdated
| let module = dr::load_bytes(blob).unwrap(); | ||
| let _disasm = module.disassemble(); | ||
| let _assembly = module.assemble(); | ||
| //TODO: unlock when ready |
There was a problem hiding this comment.
What's this TODO for? The following line is not commented out?
| }; | ||
|
|
||
| use std::path::PathBuf; | ||
|
|
||
| fn test_spv(blob: &[u8]) { | ||
| let module = dr::load_bytes(blob).unwrap(); | ||
| let _disasm = module.disassemble(); |
There was a problem hiding this comment.
I'm curious about the leading _ in names. Is this a Rust convention?
There was a problem hiding this comment.
It's recognized by the compiler as a sign that a variable is not used.
rspirv/lift/mod.rs
Outdated
| } | ||
|
|
||
| impl LiftContext { | ||
| /// Convert a module from the data representation into structured. |
There was a problem hiding this comment.
Nit: ... structured representation.
Let's be clear (even verbose) on documentation. They are meant to help understanding. :)
rspirv/lift/mod.rs
Outdated
| .ok_or(ConversionError::MissingLabel)?, | ||
| )?;*/ | ||
| basic_blocks.push(module::BasicBlock { | ||
| //label: context.labels.append(label), |
There was a problem hiding this comment.
Can we use TODOs for unimplemented stuff?
rspirv/lift/mod.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// Error that may oocur during the convesion from the data representation |
This PR introduces the conversion routine to the "lift" module as well as the appropriate error types.
|
Oh snap, I force-pushed again, sorry! |
This is based on #86 (which is based on #71 ).
This change is