Simd i8x16.extract_lane_s instruction implementation. #802
Conversation
src/interp.cc
Outdated
| // Simd instructions with Lane operand. | ||
| // a0: input v128 value. a1: lane index. | ||
| // typename T: lane data type. | ||
| template <typename R, typename V0, typename V1, typename T> |
There was a problem hiding this comment.
Is there a need to have differing lane index types? I think this can always be int (instead of using V1).
src/interp.cc
Outdated
| // a0: input v128 value. a1: lane index. | ||
| // typename T: lane data type. | ||
| template <typename R, typename V0, typename V1, typename T> | ||
| ValueTypeRep<R> SimdExtractLane(V0 a0, V1 a1) { |
There was a problem hiding this comment.
Better name for a1? Perhaps lane or index.
src/binary-reader-interp.cc
Outdated
| @@ -195,6 +195,7 @@ class BinaryReaderInterp : public BinaryReaderNop { | |||
| wabt::Result OnTernaryExpr(wabt::Opcode opcode) override; | |||
| wabt::Result OnUnreachableExpr() override; | |||
| wabt::Result EndFunctionBody(Index index) override; | |||
| wabt::Result OnSimd1LaneImmExpr(wabt::Opcode opcode, Type type, uint32_t value) override; | |||
There was a problem hiding this comment.
Why is this called Simd1LaneImm? I suppose it will be used for extract and replace lane, is that why?
There was a problem hiding this comment.
Yes, extract and replace lane only have 1 lane immediate parameter. But v8x16.shuffle have 16 lane Immediate parameters.
src/binary-reader-logging.cc
Outdated
| @@ -353,6 +353,11 @@ Result BinaryReaderLogging::OnTryExpr(Index num_types, Type* sig_types) { | |||
| return reader_->OnTryExpr(num_types, sig_types); | |||
| } | |||
|
|
|||
| Result BinaryReaderLogging::OnSimd1LaneImmExpr(Opcode opcode, Type type, uint32_t value) { | |||
| LOGF("OnSimdLaneExpr (lane: %d)\n", value); | |||
There was a problem hiding this comment.
Match function name here: OnSimd1LaneImmExpr
There was a problem hiding this comment.
Sorry for the omission. I will fix it, Thanks!
src/interp.cc
Outdated
| @@ -2414,6 +2434,13 @@ Result Thread::Run(int num_instructions) { | |||
| break; | |||
| } | |||
|
|
|||
| case Opcode::I8X16ExtractLaneS: { | |||
| v128 dLane = static_cast<v128>(Pop<v128>()); | |||
There was a problem hiding this comment.
Better name than dLane here? Other examples in this file use value.
src/ir.h
Outdated
| @@ -266,6 +267,18 @@ typedef OpcodeExpr<ExprType::Convert> ConvertExpr; | |||
| typedef OpcodeExpr<ExprType::Unary> UnaryExpr; | |||
| typedef OpcodeExpr<ExprType::Ternary> TernaryExpr; | |||
|
|
|||
| template <ExprType TypeEnum, typename T> | |||
There was a problem hiding this comment.
No need for template param T here, the lane index will always be < 16. Using int instead is sufficient.
There was a problem hiding this comment.
Consider the future v8x16.shuffle implementation.
There was a problem hiding this comment.
I see, though I'd say that v8x16.shuffle is different enough to warrant a diferrent Expr type.
There was a problem hiding this comment.
Done.
I will use a different Expr type for v8x16.shuffle.
src/wast-parser.cc
Outdated
|
|
||
| switch (opcode) { | ||
| case Opcode::I8X16ExtractLaneS: { | ||
| if(const_->v128_bits.v[0] >= 16) |
There was a problem hiding this comment.
Better to do this check in the validator, so it can be shared when reading text or binary.
There was a problem hiding this comment.
Ok, I will move the check into validator.
Is it ok to omit such type of error in parsing stage?
There was a problem hiding this comment.
Yes, I think so. Any module parsed via text will have to be validated before it can be used in most cases anyway.
src/wast-parser.cc
Outdated
| Token token = Consume(); | ||
| ErrorUnlessOpcodeEnabled(token); | ||
| Const const_; | ||
| CHECK_RESULT((ParseSimdConstWithCheck<uint32_t>(token.opcode(), Type::I32, |
There was a problem hiding this comment.
Why parse as SimdConst? This should just use ParseNat to parse a natural number.
There was a problem hiding this comment.
Still Consider the future v8x16.shuffle implementation.
ParseNat can only parse 1 natural number. But ParseSimdConst can parse several natural numbers according to the input byte numbers.
There was a problem hiding this comment.
OK, this seems a bit overly-generic to me, but I'm OK with this direction for now. We can always refactor later if necessary.
There was a problem hiding this comment.
Done.
use ParsrNat for simd extract/replace lane as v8x16.shuffle will be another Expr type.
Including: 1. All necessary code for SIMD lanes accessing. 2. i8x16.extract_lane_s implementation.
lizhengxing
force-pushed
the
simd-i8x16-extract-lane-impl-PR
branch
from
4f81159 to
8944fe9
Compare
lizhengxing
force-pushed
the
simd-i8x16-extract-lane-impl-PR
branch
2 times, most recently
from
e5c1db6 to
d85e441
Compare
Including: