Add support for pairwise_widening_add in wasm #5850
Conversation
src/CodeGen_WebAssembly.cpp
Outdated
| case VectorReduce::Add: | ||
| accumulator += v; | ||
| break; | ||
| case VectorReduce::Min: |
There was a problem hiding this comment.
There is no intrinsic for this case is there? Maybe this should just be internal_assert(op->op == VectorReduce::Add) ?
There was a problem hiding this comment.
Yeah, copypasta from CodegenARM and I was lazy about cutting it down. Will do.
src/CodeGen_WebAssembly.cpp
Outdated
| const char *intrin = nullptr; | ||
| std::vector<Expr> intrin_args; | ||
| Expr accumulator = init; | ||
| if (op->op == VectorReduce::Add && factor == 2) { |
There was a problem hiding this comment.
I think it would be better to do this with a pattern, even if there is only one.
There was a problem hiding this comment.
OK, but FWIW, this was taken directly from the existing code in Codegen_ARM, which doesn't use a pattern here. (Maybe I should backport it there afterwards...)
There was a problem hiding this comment.
(the source in Codegen_ARM has the comment: // TODO: Move this to be patterns? The patterns are pretty trivial, but some of the other logic is tricky, which got lost in translation.)
test/correctness/simd_op_check.cpp
Outdated
| }; | ||
|
|
||
| check("i16x8.extadd_pairwise_i8x16_s", 8 * w, sum_(i16(in_i8(f * x + r)))); | ||
| check("i16x8.extadd_pairwise_i8x16_u", 8 * w, sum_(u16(in_u8(f * x + r)))); |
There was a problem hiding this comment.
I think we should also check for signed widening from unsigned operands, e.g.: https://github.com/halide/Halide/blob/master/src/CodeGen_ARM.cpp#L1139
There was a problem hiding this comment.
There isn't a wasm instruction that does this -- just i8->i16 and u8->u16 (and wider variants).
I guess we could just use the u8->u16 op in this case and cast the result to i16, on the assumption that the u16 result will always have a zero sign bit for 128-bit vectors....
There was a problem hiding this comment.
Yes, that's all the ARM and other targets are doing anyways. I'm not actually sure you need to change the implementation at all, it might just work (and the ARM version might be redundant too), but I think we should test for it.
|
PTAL |
|
Monday Morning Review Ping |
|
|
||
| namespace Halide { | ||
| namespace Internal { | ||
|
|
||
| using std::string; |
There was a problem hiding this comment.
Did you mean to change this here? Most of the other backends do this (and don't quality string with std::).
There was a problem hiding this comment.
There was a mixture of usage so I normalize to std:: since that is the pattern in most non-backend code. Will normalize the other way if that's the unspoken standard here.
src/CodeGen_WebAssembly.cpp
Outdated
| int factor; | ||
| Expr pattern; | ||
| const char *intrin; | ||
| Type narrow_type; |
There was a problem hiding this comment.
I don't think you need this, all of the patterns have the narrow type the same as the pattern's type.
There was a problem hiding this comment.
At the moment, yes, but I'm not sure if that will be the case for everything, and I wanted to preserve this logic (adapted from Codegen_X86.cpp). That said, easy enough to restore if/when it's needed.
There was a problem hiding this comment.
I think we should just add it when needed. It adds a fair bit of code that is currently dead. Note that CodeGen_ARM doesn't have either of these.
src/CodeGen_WebAssembly.cpp
Outdated
| Expr pattern; | ||
| const char *intrin; | ||
| Type narrow_type; | ||
| uint32_t flags; |
There was a problem hiding this comment.
You don't need this either, no patterns use any flags.
There was a problem hiding this comment.
See above, ditto.
test/correctness/simd_op_check.h
Outdated
| while (getline(asm_file, line)) { | ||
| msg << line << "\n"; | ||
| { | ||
| std::ifstream asm_file; |
There was a problem hiding this comment.
What actually changed here?
There was a problem hiding this comment.
code restructuring that no longer matters. Will revert.
* Add support for widening_mul in wasm
No description provided.