Skip to content
This repository has been archived by the owner on Dec 22, 2021. It is now read-only.

Fix ext mul opcodes in BinarySIMD.md #406

Closed
wants to merge 2 commits into from

Conversation

ngzhian
Copy link
Member

@ngzhian ngzhian commented Dec 17, 2020

No description provided.

@ngzhian ngzhian requested a review from tlively December 17, 2020 06:34
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

I really hope that’s not permanent, but probably good to have the docs reflect reality.

@ngzhian
Copy link
Member Author

ngzhian commented Dec 21, 2020

I really hope that’s not permanent

Wdym?

@tlively
Copy link
Member

tlively commented Dec 21, 2020

It would be nice to have consistently contiguous opcodes for these instructions eventually. My understanding is that the current opcodes were necessary to prototype them quickly without increasing opcode size, so right now I would be sad if these were the final opcodes.

@Maratyszcza
Copy link
Contributor

It seems that no other WAsm engine implements these instructions yet, but it wouldn't be for long, so suggest you change the opcodes now or leave them as is. Later renumbering is always a pain.

@lars-t-hansen
Copy link
Contributor

It seems that no other WAsm engine implements these instructions yet, but it wouldn't be for long, so suggest you change the opcodes now or leave them as is. Later renumbering is always a pain.

Yes please. Either renumber ASAP or freeze until we do a final renumbering (which IMO we should not, but I lost the discussion last time and I expect to lose it again...)

@dtig
Copy link
Member

dtig commented Jan 5, 2021

IMO it's too late to do a large opcode renumbering, and the ones that were decided on in #209 should not change as was the intent of that change, and it is a significant amount of churn to do that again. Going forward though, for the open PRs, we should make sure they fit into the current opcode space in a coherent way, i.e. as the prototype opcodes are not yet merged these are not set in stone.

As we've left space for some open PRs back then, and I'm assuming a lot of the inconsistency is due to prototype opcodes (@tlively correct me if I'm wrong here), I'm hoping that within a short period of time we make a decision as to which operations will make it into the final proposal and do the best we can with numbering. before merging the ones currently being prototyped.

@tlively
Copy link
Member

tlively commented Jan 5, 2021

The only instructions we've merged to the proposal that weren't accounted for in the last renumbering are the load zero instructions, the floating point rounding instructions, and these extending multiply instructions. The load zero instructions and the extending multiply instructions were just added at the end of the opcode space, so I don't think there's a pressing reason to change them. The floating point rounding instructions were placed in the space reserved for i64x2 operations including max_s, max_u, dot_i32x4_s, and avgr_u. As long as we don't plan on introducing any of those i64x2 instructions, those can probably stay where they are as well.

I think the best move would be to update the extending multiply instruction implementations to match the current spec proposal (i.e. have them use ops 0x110-0x11b). For any future instructions that we merge to the proposal, we should just add them at the end rather than putting them in holes reserved for other instructions. I think the thing blocking this plan is that V8 does not support opcodes above 0xff right now. @ngzhian how hard would that be to support?

@ngzhian
Copy link
Member Author

ngzhian commented Jan 6, 2021

I'm hoping that within a short period of time we make a decision as to which operations will make it into the final proposal and do the best we can with numbering. before merging the ones currently being prototyped.

I agree with Deepti. We have a number of outstanding proposals, especially i64x2 ones. I think we should figure out if we want those or not, then worry about renumbering.

  • if we don't include them, we can likely fit everything into 0xff opcodes, so this won't be a problem
  • if we include them, we will exceed 0xff, and V8 will have to support it

how hard would that be to support?

This is a cross cutting change, it wouldn't be hard, but will be many lines changed.

@tlively
Copy link
Member

tlively commented Jan 6, 2021

My understanding of the proposed plan is to first figure out all the additional instructions we intend to add to the instruction set, then to figure out how best number them. No instructions that have already been merged at this point (except possibly the extending multiply instructions) will have their numbers changed. @ngzhian @dtig is that along the lines of what you were thinking?

@dtig
Copy link
Member

dtig commented Jan 6, 2021

@tlively Yes, that is what I was thinking. As we are close to the end here, I like the idea of evaluating all the outstanding instructions and make a final decision - it makes the boundary between prototype/officially supported opcodes clear as well. As @ngzhian mentioned, it's not hard to support but we would rather not if it's not required.

@ngzhian
Copy link
Member Author

ngzhian commented Feb 9, 2021

Deprecating this, we will fix all opcodes after #452 .

@ngzhian ngzhian closed this Feb 9, 2021
@ngzhian ngzhian deleted the fix-ext-mul-opcodes branch February 9, 2021 20:34
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.

5 participants