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

Rework the API for outgoing blockparams #18

Merged
merged 2 commits into from
Dec 1, 2021

Conversation

Amanieu
Copy link
Contributor

@Amanieu Amanieu commented Nov 30, 2021

Branch parameters are not proper operands, so it doesn't make sense to represent them using Operands. The constraint is irrelevant (you always want Any), and so is the position within the instruction (early/late).

This PR adds Function::branch_params which returns the branch parameters directly as a list of VRegs.

This is also useful for rematerialization (#8): branch parameters don't need to be in an actual register or stack slot if we can rematerialize them later. This would normally be required by the Any constraint used for branch parameters.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Thanks for this! I was unsure at first about this refactor (the appeal of the blockparams approach to me is that phis turn into normal operands at their points of logical use, but... now they aren't operands?) but I think this is actually the right abstraction, on further thought: blockparams are indeed differently-constrained (they aren't really a use that must read the value, they just plumb it through), and the scheme for packing them contiguously in the args of a branch instruction was awkward. And blockparam inputs are special, so why not blockparam outputs too?

IIRC, an earlier iteration of RA2 actually depended on the uses existing, but the current implementation of blockparam plumbing makes use of the unified move generation so I think everything should work as it is now. Just to check: did you run the fuzzer for a bit with the updated logic?

@Amanieu
Copy link
Contributor Author

Amanieu commented Nov 30, 2021

I did run the fuzzer, but looking at the code again I think that the live ranges are not being tracked properly. I need to double-check this.

@cfallin
Copy link
Member

cfallin commented Nov 30, 2021

Ah, yes, the dataflow analysis around liveranges.rs line 333 needs to consider out-blockparams as well, I think.

@Amanieu
Copy link
Contributor Author

Amanieu commented Dec 1, 2021

Fixed, but I'll leave the fuzzer running overnight again to be sure.

@Amanieu
Copy link
Contributor Author

Amanieu commented Dec 1, 2021

Fuzz tested ~4M test cases and no problems found! But then again fuzzing was also passing without the liverange fixes...

@cfallin
Copy link
Member

cfallin commented Dec 1, 2021

Sounds good. I'm not sure why the fuzzer didn't catch that -- perhaps it's likely to cover liveranges anyway by randomly using lots of values from earlier in the program... in any case, the logic looks correct now.

@cfallin cfallin merged commit 822f2bc into bytecodealliance:main Dec 1, 2021
@Amanieu Amanieu deleted the blockparam branch November 24, 2023 04:42
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

Successfully merging this pull request may close these issues.

2 participants