Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Bitcasting at control flow exits #1272

Merged

Conversation

abrown
Copy link
Collaborator

@abrown abrown commented Dec 4, 2019

  • This has been discussed in issue #1192.
  • A short description of what this does, why it is needed: I have other PRs (Relax verification to allow I8X16 to act as a default vector type #1236 and Add X128 type to represent WebAssembly's V128 type #1251) to solve the underlying SIMD type issues in different ways; however, they still fall short because I have not yet found a good way to retain the necessary type information to inform cranelift that, e.g., iadd needs to operate on i32x4 and not the default i8x16. This PR adds more raw_bitcasts so that function signatures returning vectors will have some bitcasts before return to convert the actual vector type (e.g. i32x4) into the expected signature type (e.g. the default i8x16). I propose we merge something like this first so I can get more SIMD spec tests running and circle back to a better solution to #1192 afterwards.
  • This PR contains test cases, if meaningful.
  • A reviewer from the core maintainer team has been assigned for this PR.

state.peekn(return_count),
&builder.func.signature.returns.clone(), // TODO this use of signature
// returns may be incorrect; what if this End is in a different control flow
// structure?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR is still a WIP because 1) I'm not yet confident that I've covered all of the possible cases where a vector should be bitcast to another type and 2) in situations like the above (Operator::End), I am pretending that I can bitcast to the function return types but in fact the End may be the end of some other block, not necessarily the end of a function (though adding this makes more SIMD spec tests pass).

Copy link
Member

Choose a reason for hiding this comment

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

I think this might be on the right track, except that you should use the block's signature, rather than the function's signature.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

The approach in the code so far is that operations bitcast their inputs as needed, which I think makes sense.

To do this for the function exits, we should do it at the point of the return_ calls, both the one for the Return opcode as you have here, but also the one in func_translator.rs, which is the return that gets inserted at the end of a function to handle the case where no Return opcode was present at end.

That leaves the question of control-flow merge points.

state.peekn(return_count),
&builder.func.signature.returns.clone(), // TODO this use of signature
// returns may be incorrect; what if this End is in a different control flow
// structure?
Copy link
Member

Choose a reason for hiding this comment

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

I think this might be on the right track, except that you should use the block's signature, rather than the function's signature.

cranelift-wasm/src/code_translator.rs Outdated Show resolved Hide resolved
@abrown
Copy link
Collaborator Author

abrown commented Dec 10, 2019

@sunfishcode, I pushed some commits that should resolve your comments above. I am still thinking through your comment about control flow merge points: shouldn't control flow divergences merge back into a common EBB? If that is the case, I think my 114ca62 commit might solve that problem--I look at the parameters of the EBB we jump to at an Operator::End and bitcast any vectors to the expected type in that next EBB.

But perhaps you meant something else: I believe we will have to bitcast in some way in all the cases listed as control instructions in the spec; e.g., before a call we will have to bitcast to I8X16 which is what the called function will expect type-wise. Is that more what you meant?

@abrown abrown force-pushed the bitcasting-at-control-flow-exits branch from cdf17d3 to 0cd40e0 Compare December 11, 2019 19:38
@abrown abrown marked this pull request as ready for review December 11, 2019 21:58
@abrown abrown force-pushed the bitcasting-at-control-flow-exits branch 2 times, most recently from 963ceb6 to 4707e13 Compare December 11, 2019 22:01
@abrown
Copy link
Collaborator Author

abrown commented Dec 11, 2019

@sunfishcode, I think this is ready for another look. I am open to adding any new test cases you can think of (perhaps simd-call.wat and simd-icall.wat should be merged into call.wat and icall.wat?).

@bnjbvr bnjbvr removed their request for review December 12, 2019 09:32
@bnjbvr
Copy link
Member

bnjbvr commented Dec 12, 2019

(Unflagging me for review, since @sunfishcode is already flagged in and did review a first time.)

@abrown abrown force-pushed the bitcasting-at-control-flow-exits branch from 4707e13 to 03b0fcb Compare December 13, 2019 00:45
@abrown abrown force-pushed the bitcasting-at-control-flow-exits branch from 03b0fcb to 9657262 Compare December 13, 2019 01:11
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for seeing this through!

@sunfishcode sunfishcode merged commit 111465f into bytecodealliance:master Jan 6, 2020
@abrown abrown deleted the bitcasting-at-control-flow-exits branch January 7, 2020 18:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants