-
Notifications
You must be signed in to change notification settings - Fork 57
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
Structured module items #63
Conversation
This needs a rebase now. :) |
Rebased now |
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.
LGTM!
I just have a few comments. Feel free to submit after addressing them.
Please make sure the commit message is edited properly to show the
changes. Thanks!
function_type: spirv::Word, | ||
} | ||
#[derive(Clone, Debug, Eq, PartialEq)] | ||
pub struct FunctionParameter {} |
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.
Do we still want to have separate ops for these function components? I think they should be folded into the function SR itself. I mean, FunctionEnd
is useful for marking the end of a function in SPIR-V binary, but not useful for SR. Similarly for Label
, it should be implicit for BasicBlock SR. Letting each pass processing Function or BasicBlock to worry about FunctionEnd
and Label
is not helpful.
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 agree, we'll need to set up a list of ops that don't need SR autogen generation, and which we are going to be converting to and from DR manually. We'll do it as a follow-up.
rspirv/sr/items.rs
Outdated
/// | ||
/// Although it is required by the specification to appear exactly once | ||
/// per module, we keep it optional here to allow flexibility. | ||
pub memory_model: Option<op::MemoryModel>, |
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.
Do we want to still keep it optional in SR? I think we should mandate it here.
rspirv/sr/items.rs
Outdated
|
||
pub struct Module { | ||
/// The module header. | ||
pub header: Option<ModuleHeader>, |
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 be mandated?
//pub interface: Vec<spirv::Word>, | ||
} | ||
|
||
pub struct BasicBlock { |
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.
Where is the Vector for instructions?
Thanks for the review! All the concerns are addressed now, I believe. |
I think the commit message is descriptive enough? If not, let's talk about this some more and figure out good guidelines. |
I typically prefer to have a concise onliner for describing what the commit is doing. The onliner shouldn't be too long so it renders nicely on GitHub UI (otherwise we will see ... and broken sentences) and also CLI log dump. Then have some more detailed explanation following after an empty line. I'd prefer to have the detailed explanation to have proper line breaks so each paragraph is not on a single line. (Otherwise when we do |
The content of the message is good (modulo a typo for |
This is another small step towards #5 , taken out of #46
It's based on #62, so only the second commit needs to be reviewed here.
We are now generating a few enums and a few standalone structs for the instructions.