Wasmtime GitHub notifications bot (Sep 27 2021 at 20:17): Wasmtime GitHub notifications bot (Sep 27 2021 at 20:17):cfallin commented on issue #3391:
Hi @bjorn3 -- thanks for the PR; a few questions and thoughts:
Can you say more about your intended use-case for this? Did this approach arise to solve a specific problem or are you wanting to expand the array of backends in general?
The maintenance burden of adding a new backend is nontrivial; in this case, both when CLIF changes, on the input side of this translator, and when LLVM's API changes, on the output side of this translator. It is also another translation layer to test in some way, and another potential source of bugs. With that in mind, I'm somewhat concerned about the prospect of accepting this, unless or until a compelling use-case arises for which we don't have a better answer.
This potentially also spawns some conversations about our goals and vision w.r.t. compiler backends. We've been gradually steering in a direction to where we have a focus in Cranelift on correctness (especially with recent efforts on different kinds of fuzzing, and with allowance for verification efforts in ISLE), while maintaining a reasonable level of speed, and in general adding multiple compiler frameworks might diffuse the efforts toward those goals. This point definitely deserves further discussion, but at least right now I would prefer to try to solve issues and needs by addressing them within the main Cranelift infrastructure, unless there is clearly a very different use-case that can't work that way.
Given the above, would it be a possibility for this translator to be maintained in a separate repository? It looks to me like this crate basically consumes the public CLIF definitions in order to translate CLIF, so I suspect nothing would technically prevent this. There is of course the overhead of tracking changes, and we don't guarantee the stability of Cranelift APIs (only Wasmtime APIs) in this repo, but as mentioned above this work has to happen either way; the question is just where the management burden lies.
Thoughts? I'm interested to hear what others think as well!
Wasmtime GitHub notifications bot (Sep 27 2021 at 20:20):cfallin edited a comment on issue #3391:
Hi @bjorn3 -- thanks for the PR; a few questions and thoughts:
Can you say more about your intended use-case for this? Did this approach arise to solve a specific problem or are you wanting to expand the array of backends in general?
The maintenance burden of adding a new backend is nontrivial; in this case, both when CLIF changes, on the input side of this translator, and when LLVM's API changes, on the output side of this translator. It is also another translation layer to test in some way, and another potential source of bugs. With that in mind, I'm somewhat concerned about the prospect of accepting this, unless or until a compelling use-case arises for which we don't have a better answer.
This potentially also spawns some conversations about our goals and vision w.r.t. compiler backends. We've been gradually steering in a direction to where we have a focus in Cranelift on correctness (especially with recent efforts on different kinds of fuzzing, and with allowance for verification efforts in ISLE), while maintaining a reasonable level of speed, and in general adding multiple compiler frameworks might diffuse the efforts toward those goals. This point definitely deserves further discussion, but at least right now I would prefer to try to solve issues and needs by addressing them within the main Cranelift infrastructure, unless there is clearly a very different use-case that can't work that way.
Given the above, would it be a possibility for this translator to be maintained in a separate repository? It looks to me like this crate basically consumes the public CLIF definitions in order to translate CLIF, so I suspect nothing would technically prevent this. There is of course the overhead of tracking changes, and we don't guarantee the stability of Cranelift APIs (only Wasmtime APIs) in this repo, but as mentioned above this work has to happen either way; the question is just where the maintenance burden lies.
Thoughts? I'm interested to hear what others think as well!
Wasmtime GitHub notifications bot (Sep 27 2021 at 20:29):bjorn3 commented on issue #3391:
Can you say more about your intended use-case for this? Did this approach arise to solve a specific problem or are you wanting to expand the array of backends in general?
A combination of because I can, because it helps with detecting cases where I depend on the fact that Cranelift doesn't optimize a lot (I already found two in cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/1199 I am curious how much breaks in cg_clif when enabling more optimizations.) and because it could be useful when you want high runtime performance without duplicating a backend for fast unoptimized compilation. It could also be used to prototype clif ir changes for new features before the regular cranelift backends gain support for it to eg see if the changes don't put undue burden on clif ir producers. It also seems that cg_clif + this backend produces faster code than cg_llvm in debug mode despite cg_llvm using more optimization passes. Didn't benchmark compilation times though.
The maintenance burden of adding a new backend is nontrivial
I see
Given the above, would it be a possibility for this translator to be maintained in a separate repository?
That would be an option.
Wasmtime GitHub notifications bot (Sep 27 2021 at 20:33):bjorn3 commented on issue #3391:
because it helps with detecting cases where I depend on the fact that Cranelift doesn't optimize a lot (I already found two in cg_clif: bjorn3/rustc_codegen_cranelift#1199 I am curious how much breaks in cg_clif when enabling more optimizations.)
This may actually help with the correctness goal of Cranelift by finding inaccuracies between the documented semantics of clif ir around for example the memory model and the actual implementation in the backends. As we don't have a lot of optimizations yet it is very easy for violations of the memory model to compile to something that works anyway. The memory model of clif ir includes flags like readonly, notrap and aligned.
Wasmtime GitHub notifications bot (Sep 28 2021 at 20:37):cfallin commented on issue #3391:
@bjorn3 Interesting -- thanks for elaborating on the uses a bit here! While the differential testing possibilities that this could allow may be nice in some ways, I think overall the maintenance overhead (and the overhead of testing the translator itself) tips the balance toward keeping this outside the tree, in its own repository, for now. In the future if it becomes a very useful or popular option then of course this can be reconsidered but maybe not today, with our existing resources. Does that seem reasonable to you?
Wasmtime GitHub notifications bot (Sep 28 2021 at 20:47):bjorn3 commented on issue #3391:
Sure, that is reasonable.
Wasmtime GitHub notifications bot (Jun 11 2023 at 00:25):Anutrix commented on issue #3391:
Was this ever moved to a separate repository?
Wasmtime GitHub notifications bot (Jun 11 2023 at 06:49):bjorn3 commented on issue #3391:
No it was not, but I intentionally didn't delete the branch of this PR to ensure it can be picked up again.
Last updated: Nov 22 2024 at 17:03 UTC