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

No way to make forward references with mr::Builder #7

Closed
rrika opened this issue Feb 19, 2017 · 3 comments
Closed

No way to make forward references with mr::Builder #7

rrika opened this issue Feb 19, 2017 · 3 comments

Comments

@rrika
Copy link

rrika commented Feb 19, 2017

When building an if-else–construct using branch_conditional(cond, true_label, false_label, …) I need the OpLabel-ids before they are generated.

If branch_conditional returned &mr::Instruction and forward references were Cell<Word> instead of Word I could patch it later. Though this would allow anyone with a non-mut reference to mess things up.

@rrika
Copy link
Author

rrika commented Feb 19, 2017

Never mind, I should’ve read the documentation. Though even if I use builder.id() how do I make a later basic block use exactly that id instead of the next auto-generated one? The builder gives no mutable references to any of the objects it creates, as far as I can tell.

@antiagainst
Copy link
Collaborator

Yep, confirmed. This is a flaw in the current builder API. I knew forward references would bite me so I left some comments in the doc about it. However, the solution covers some case but not all. I'll fix the rest soon. :)

Returning an instruction to allow fixing up things later was truly the way I'd like to take previously. But after a second thought, wrapping all instructions with Cell introduces too much overheads into the current data representation. So I think I'll instead change the builder methods to take an additional Option<spirv::Word> for result ids. It brings in some boilerplate code when using the API, but it keeps the current simple data representation.

Thanks for reporting! :)

@antiagainst
Copy link
Collaborator

Fixed. I'll push a new release after adding some more tests. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants