Wasmtime GitHub notifications bot (May 22 2020 at 19:16): Wasmtime GitHub notifications bot (May 22 2020 at 19:16):akirilov-arm opened PR #1748 from simd_store to master:
This PR is the first step in implementing SIMD support for AArch64. Since it is also my first Cranelift patch, there are a couple of things that are not clear to me, and I think that it is best to discuss them here:
- I noticed that the AArch64 backend used to support constant pools, but that functionality was removed. It was used only for integer constants, which can be materialized efficiently with arithmetic instructions. However, that's not really the case with vector constants; furthermore, if the same vector values are used frequently, the code size penalty with my current implementation is much worse than the one for smaller data types. Does it make sense to restore the constant pool functionality for vectors?
- I couldn't figure out another way to get the ir::Function that was being compiled from the lowering context, so I added a method, but I am definitely open to better suggestions.
- The intent behind the
InstSizeenum is not clear to me (and the name should probably be changed as well) - it looks like it is meant for integer operands, but is already used by floating-point operations. Does it make sense to refactor that code? For now I made the minimal changes to pass the test, but I added aTODOto highlight the issue.- All SIMD tests are ignored for AArch64, which means that actual testing of this patch requires manual changes to
build.rs. I could add the necessary one line change, but since we (Arm) plan on enabling more SIMD tests, that would become a bit awkward. Any opinions?
Wasmtime GitHub notifications bot (May 22 2020 at 19:17):akirilov-arm edited PR #1748 from simd_store to master:
This PR is the first step in implementing SIMD support for AArch64. Since it is also my first Cranelift patch, there are a couple of things that are not clear to me, and I think that it is best to discuss them here:
- I noticed that the AArch64 backend used to support constant pools, but that functionality was removed. It was used only for integer constants, which can be materialized efficiently with arithmetic instructions. However, that's not really the case with vector constants; furthermore, if the same vector values are used frequently, the code size penalty with my current implementation is much worse than the one for smaller data types. Does it make sense to restore the constant pool functionality for vectors?
- I couldn't figure out another way to get the
ir::Functionthat was being compiled from the lowering context, so I added a method, but I am definitely open to better suggestions.- The intent behind the
InstSizeenum is not clear to me (and the name should probably be changed as well) - it looks like it is meant for integer operands, but is already used by floating-point operations. Does it make sense to refactor that code? For now I made the minimal changes to pass the test, but I added aTODOto highlight the issue.- All SIMD tests are ignored for AArch64, which means that actual testing of this patch requires manual changes to
build.rs. I could add the necessary one line change, but since we (Arm) plan on enabling more SIMD tests, that would become a bit awkward. Any opinions?
Wasmtime GitHub notifications bot (May 22 2020 at 19:20):akirilov-arm updated PR #1748 from simd_store to master:
This PR is the first step in implementing SIMD support for AArch64. Since it is also my first Cranelift patch, there are a couple of things that are not clear to me, and I think that it is best to discuss them here:
- I noticed that the AArch64 backend used to support constant pools, but that functionality was removed. It was used only for integer constants, which can be materialized efficiently with arithmetic instructions. However, that's not really the case with vector constants; furthermore, if the same vector values are used frequently, the code size penalty with my current implementation is much worse than the one for smaller data types. Does it make sense to restore the constant pool functionality for vectors?
- I couldn't figure out another way to get the
ir::Functionthat was being compiled from the lowering context, so I added a method, but I am definitely open to better suggestions.- The intent behind the
InstSizeenum is not clear to me (and the name should probably be changed as well) - it looks like it is meant for integer operands, but is already used by floating-point operations. Does it make sense to refactor that code? For now I made the minimal changes to pass the test, but I added aTODOto highlight the issue.- All SIMD tests are ignored for AArch64, which means that actual testing of this patch requires manual changes to
build.rs. I could add the necessary one line change, but since we (Arm) plan on enabling more SIMD tests, that would become a bit awkward. Any opinions?
Wasmtime GitHub notifications bot (May 22 2020 at 19:43):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (May 22 2020 at 19:43):bjorn3 created PR Review Comment:
Maybe only expose
func.dfg.constantsfromctx?
Wasmtime GitHub notifications bot (May 22 2020 at 22:50):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (May 22 2020 at 22:50):cfallin created PR Review Comment:
Sure, that would make sense. The type is currently used in places where 32-bit and 64-bit variants of an instruction exist; so a separate 64/128-bit (or 32/64/128-bit?) version for FP/SIMD instructions that take on those sizes would make sense. (In other words, avoid allowing options that don't make sense for the particular instruction.)
Wasmtime GitHub notifications bot (May 22 2020 at 22:50):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (May 22 2020 at 22:50):cfallin created PR Review Comment:
Yes, agreed; there's a (possibly not adequately documented) goal of insulating the lowering code from the raw CLIF input as much as possible, to give us flexibility in the future. (E.g., we might someday use a VCode container of
InstructionDatas to carry non-SSA code directly from the wasm lowering; we'd need a little plumbing, but the trait abstraction gets us most of the way there.) Something likectx.get_constant_data(constant_handle)would make sense, IMHO.
Wasmtime GitHub notifications bot (May 22 2020 at 22:51):cfallin assigned PR #1748 to cfallin.
Wasmtime GitHub notifications bot (May 24 2020 at 22:01):akirilov-arm submitted PR Review.
Wasmtime GitHub notifications bot (May 24 2020 at 22:01):akirilov-arm created PR Review Comment:
Do you mind if that happens in a separate patch as well?
Wasmtime GitHub notifications bot (May 26 2020 at 12:40):akirilov-arm updated PR #1748 (assigned to cfallin) from simd_store to master:
This PR is the first step in implementing SIMD support for AArch64. Since it is also my first Cranelift patch, there are a couple of things that are not clear to me, and I think that it is best to discuss them here:
- I noticed that the AArch64 backend used to support constant pools, but that functionality was removed. It was used only for integer constants, which can be materialized efficiently with arithmetic instructions. However, that's not really the case with vector constants; furthermore, if the same vector values are used frequently, the code size penalty with my current implementation is much worse than the one for smaller data types. Does it make sense to restore the constant pool functionality for vectors?
- I couldn't figure out another way to get the
ir::Functionthat was being compiled from the lowering context, so I added a method, but I am definitely open to better suggestions.- The intent behind the
InstSizeenum is not clear to me (and the name should probably be changed as well) - it looks like it is meant for integer operands, but is already used by floating-point operations. Does it make sense to refactor that code? For now I made the minimal changes to pass the test, but I added aTODOto highlight the issue.- All SIMD tests are ignored for AArch64, which means that actual testing of this patch requires manual changes to
build.rs. I could add the necessary one line change, but since we (Arm) plan on enabling more SIMD tests, that would become a bit awkward. Any opinions?
Wasmtime GitHub notifications bot (May 26 2020 at 16:19):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (May 26 2020 at 16:19):cfallin created PR Review Comment:
Yep, that's fine!
Wasmtime GitHub notifications bot (May 26 2020 at 16:21):cfallin merged PR #1748.
Last updated: Dec 23 2024 at 12:05 UTC