Skip to content

Conversation

@tlively
Copy link
Member

@tlively tlively commented Sep 3, 2019

Renames the SIMDBitselect class to SIMDTernary and adds the new
{f32x4,f64x2}.qfm{a,s} ternary instructions. Because the SIMDBitselect
class is no more, this is a backwards-incompatible change to the C
interface. The new instructions are not yet used in the fuzzer because
they are not yet implemented in V8.

The corresponding LLVM commit is https://reviews.llvm.org/rL370556.

Renames the SIMDBitselect class to SIMDTernary and adds the new
{f32x4,f64x2}.qfm{a,s} ternary instructions. Because the SIMDBitselect
class is no more, this is a backwards-incompatible change to the C
interface. The new instructions are not yet used in the fuzzer because
they are not yet implemented in V8.
@tlively tlively requested review from aheejin and kripken September 3, 2019 18:19
CHANGELOG.md Outdated
- wasm-emscripten-finalize: Don't rely on name section being present in the
input. Use the exported names for things instead.
- Added `mutable` parameter to BinaryenAddGlobalImport.
- Replace BinarySIMDBitselect* with BinarnSIMDTernary* in the C API and add
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Replace BinarySIMDBitselect* with BinarnSIMDTernary* in the C API and add
- Replace BinarySIMDBitselect* with BinarySIMDTernary* in the C API and add

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow this is all kinds of wrong. Thanks for the catch!

Copy link
Member

Choose a reason for hiding this comment

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

Oh Binaryen* makes much more sense :)

Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT switch to more general name makeTernary? According to FutureFeatures.md WebAssembly probably introduce fused multiply add instructions for scalar as well or only SIMD planning?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that renaming to Ternary would make sense if we ever get ternary scalar operations. We could even rename Select to Ternary and getting rid of the separate SIMDTernary altogether, which would be consistent with how we treat SIMD unary and binary ops. I don't know of any concrete plans to add new scalar ternary operations, though, and I think keeping the separate SIMD instruction type is useful for now because it simplifies the common case in which SIMD is not considered.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Great!

Expression* cond = make(v128);
return builder.makeSIMDBitselect(left, right, cond);
Expression* makeSIMDTernary() {
// SIMDTernaryOp op = pick(Bitselect,
Copy link
Member

Choose a reason for hiding this comment

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

please add TODO here

@tlively tlively merged commit 0faa68b into WebAssembly:master Sep 3, 2019
tlively pushed a commit that referenced this pull request Sep 11, 2019
In #2328 the SIMDBitselect API has been replaced with SIMDTernary that now has Bitselect as one of multiple operations, which is currently not exposed, unlike the new QFMA/QFMS operations which are exposed. This PR adds it.
@tlively tlively deleted the qfma branch April 24, 2020 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants