Wasmtime GitHub notifications bot (Aug 18 2020 at 00:45): Wasmtime GitHub notifications bot (Aug 18 2020 at 00:45):github-actions[bot] commented on Issue #2136:
Subscribe to Label Action
cc @bnjbvr
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64"Thus the following users have been cc'd because of the following labels:
- bnjbvr: cranelift
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Aug 19 2020 at 16:30):bnjbvr commented on Issue #2136:
(Sorry for the slowness, lgtm too. As a general question, what was the reasoning behind having one SseOpcode enum per lane size vs one single SseOpcode for all, and the lane size in the Vcode inst?)
Wasmtime GitHub notifications bot (Aug 19 2020 at 17:21):jlb6740 commented on Issue #2136:
(Sorry for the slowness, lgtm too. As a general question, what was the reasoning behind having one SseOpcode enum per lane size vs one single SseOpcode for all, and the lane size in the Vcode inst?)
Hi @bnjbvr .. no slowness, thanks for the review! This question on per lane vs no lane in the enum is a good one. I think we discussed it in a recent cranelift meeting and is something that can be changed in the future if we think that is best. I think we said the benefits for having one SseOpcode per lane (how it is now) were just (1) the explicitness that made the code easier to read, less to infer, less to chase down what a lowering was ultimately doing, and (2) the explicitness prevents the creation of lowering nonsensical instructions. So that was general thought, but you were out and obviously enumerating each instruction is maybe not desired and maybe removes some sort of flexibility and so ultimately this is something that is still an open and can be refactored if that's the better direction.
Wasmtime GitHub notifications bot (Aug 19 2020 at 17:22):jlb6740 edited a comment on Issue #2136:
(Sorry for the slowness, lgtm too. As a general question, what was the reasoning behind having one SseOpcode enum per lane size vs one single SseOpcode for all, and the lane size in the Vcode inst?)
Hi @bnjbvr .. no slowness, thanks for the review! This question of one instruction per lane vs no lane in the enum is a good one. I think we discussed it in a recent cranelift meeting and is something that can be changed in the future if we think that is best. I think we said the benefits for having one SseOpcode per lane (how it is now) were just (1) the explicitness that made the code easier to read, less to infer, less to chase down what a lowering was ultimately doing, and (2) the explicitness prevents the creation of lowering nonsensical instructions. So that was general thought, but you were out and obviously enumerating each instruction is maybe not desired and maybe removes some sort of flexibility and so ultimately this is something that is still an open and can be refactored if that's the better direction.
Wasmtime GitHub notifications bot (Aug 19 2020 at 17:23):jlb6740 edited a comment on Issue #2136:
(Sorry for the slowness, lgtm too. As a general question, what was the reasoning behind having one SseOpcode enum per lane size vs one single SseOpcode for all, and the lane size in the Vcode inst?)
Hi @bnjbvr .. no slowness, thanks for the review! This question of one instruction per lane vs no lane in the enum is a good one. I think we discussed it in a recent cranelift meeting and is something that can be changed in the future if we think that is best. I think we said the benefits for having one SseOpcode per lane (how it is now) were just (1) the explicitness that made the code easier to read, less to infer, less to chase down what a lowering was ultimately doing, and (2) the explicitness prevents the creation of lowering nonsensical instructions. So that was general thought, but you were out and obviously enumerating each instruction is maybe not desired/unwieldy and maybe removes some sort of flexibility and so ultimately this is something that is still an open and can be refactored if that's the better direction.
Last updated: Nov 22 2024 at 16:03 UTC