Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SIMD #1820

Merged
merged 37 commits into from Dec 14, 2018
Merged

SIMD #1820

merged 37 commits into from Dec 14, 2018

Conversation

@tlively
Copy link
Collaborator

tlively commented Dec 12, 2018

This PR has:

  1. Parsing and printing
  2. Assembling and disassembling
  3. Interpretation
  4. C API
  5. JS API

and their respective tests. It does not have:

  1. Fuzzing
@tlively tlively requested a review from kripken Dec 12, 2018
Copy link
Member

kripken left a comment

Great work!

Initial batch of comments. Overall I didn't see anything worrying so I think this is close to landing from my point of view, but I'm still reading.

explicit Literal(Type type) : type(type), i64(0) {}
explicit Literal(int32_t init) : type(Type::i32), i32(init) {}
explicit Literal(uint32_t init) : type(Type::i32), i32(init) {}
explicit Literal(int64_t init) : type(Type::i64), i64(init) {}
explicit Literal(uint64_t init) : type(Type::i64), i64(init) {}
explicit Literal(float init) : type(Type::f32), i32(bit_cast<int32_t>(init)) {}
explicit Literal(double init) : type(Type::f64), i64(bit_cast<int64_t>(init)) {}
explicit Literal(const uint8_t init[16]);
explicit Literal(const std::array<Literal, 16>&);
explicit Literal(const std::array<Literal, 8>&);

This comment has been minimized.

Copy link
@kripken

kripken Dec 13, 2018

Member

I think it's not obvious what the <16 sizes mean here. from the code I gather it means i16, i32, i64 elements, etc. - perhaps a comment would be good.


bool isConcrete() { return type != none; }
bool isNull() { return type == none; }

inline static Literal makeLiteralFromInt32(int32_t x, Type type) {

This comment has been minimized.

Copy link
@kripken

kripken Dec 13, 2018

Member

i agree the refactoring to move this code here makes sense. after that move though, I think we can remove the Literal from the name.

WASM_UNREACHABLE();
}

inline static Literal makeLiteralZero(Type type) {

This comment has been minimized.

Copy link
@kripken

kripken Dec 13, 2018

Member

I suggest the same name change here.

@@ -60,8 +88,12 @@ class Literal {
int64_t geti64() const { assert(type == Type::i64); return i64; }
float getf32() const { assert(type == Type::f32); return bit_cast<float>(i32); }
double getf64() const { assert(type == Type::f64); return bit_cast<double>(i64); }
std::array<uint8_t, 16> getv128() const;


int32_t* geti32Ptr() { assert(type == Type::i32); return &i32; } // careful!

This comment has been minimized.

Copy link
@kripken

kripken Dec 13, 2018

Member

maybe move the careful! comment to a line before this, so talks about all 3 lines

};

} // namespace wasm

namespace std {
template<> struct hash<wasm::Literal> {
size_t operator()(const wasm::Literal& a) const {
uint8_t bytes[16] = {};

This comment has been minimized.

Copy link
@kripken

kripken Dec 13, 2018

Member

do you need the = {}?

This comment has been minimized.

Copy link
@tlively

tlively Dec 13, 2018

Author Collaborator

I would not need to zero-initialize this array if I made the change to getBits that you suggest below, so this will be resolved by that discussion.

}

template<typename LaneT, int Lanes>
static void extract_bytes(uint8_t (&dest)[16], const LaneArray<Lanes>& lanes) {

This comment has been minimized.

Copy link
@kripken

kripken Dec 13, 2018

Member

we normally use cameCase (so, extractBytes) but I guess this is more standard for this style of C++. I'm fine either way.

This comment has been minimized.

Copy link
@tlively

tlively Dec 13, 2018

Author Collaborator

Seems like a fine change to make.

@@ -72,20 +118,25 @@ double Literal::getFloat() const {
}
}

int64_t Literal::getBits() const {
void Literal::getBits(uint8_t (&buf)[16]) const {
switch (type) {

This comment has been minimized.

Copy link
@kripken

kripken Dec 13, 2018

Member

i think it would be best for this method to zero out the unused bits in buf, just to be safe.

This comment has been minimized.

Copy link
@tlively

tlively Dec 13, 2018

Author Collaborator

Another option here would be to change the argument to be a raw pointer. The nice thing about doing that (and not zeroing out the rest of the 16 bytes) is that you can use getBits to write into any type that is appropriate to hold the literal's underlying value (e.g. int32_t or int64_t) rather than just an array of bytes. WDYT?

This comment has been minimized.

Copy link
@kripken

kripken Dec 13, 2018

Member

Not sure what you mean by raw pointer - of what type? (void*?)

Perhaps there could be getBits(uint8_t& x) which would write a 1-byte value, and assert the value fits into one byte, and likewise overrides with uint16& etc., but what would be the type for the simd variant, though?

This comment has been minimized.

Copy link
@tlively

tlively Dec 13, 2018

Author Collaborator

Yes, the current code now has getBits take a void*. You can see how this is used in the implementation of less at the bottom of literal.h. Overriding getBits for each size seems similar to removing the type asserts from geti32() and geti64(), which would also serve most of the same usecases. getBits would then become getv128 since it wouldn't be useful otherwise.

This comment has been minimized.

Copy link
@kripken

kripken Dec 13, 2018

Member

Oh, I'm not a fan of the void* approach... that seems too risky. Overriding for each size would ensure that we write to the proper type, which we'd lose with void*?

This comment has been minimized.

Copy link
@tlively

tlively Dec 14, 2018

Author Collaborator

This is fixed now.

@@ -1701,6 +1721,21 @@ BinaryConsts::ASTNodes WasmBinaryBuilder::readExpression(Expression*& curr) {
throwError("invalid code after nontrapping float-to-int prefix: " + std::to_string(code));
break;
}
case BinaryConsts::SIMDPrefix: {
uint32_t opcode = getU32LEB();

This comment has been minimized.

Copy link
@kripken

kripken Dec 13, 2018

Member

auto


auto ret = allocator.alloc<Const>();
auto getLiteral = [](Expression* expr) {
if (expr == nullptr) {

This comment has been minimized.

Copy link
@kripken

kripken Dec 13, 2018

Member

I'd prefer to indent this by just 2 (so the }; ends up at the bottom), but there is no right way for stuff like this I guess...

shouldBeFalse(curr->isAtomic && !getModule()->memory.shared, curr, "Atomic operation with non-shared memory");
validateMemBytes(curr->bytes, curr->valueType, curr);
validateAlignment(curr->align, curr->type, curr->bytes, curr->isAtomic, curr);
validateAlignment(curr->align, curr->valueType, curr->bytes, curr->isAtomic, curr);

This comment has been minimized.

Copy link
@kripken

kripken Dec 13, 2018

Member

oh wow, we had this wrong and it was doing nothing before, heh... nice find.

@kripken

This comment has been minimized.

Copy link
Member

kripken commented Dec 14, 2018

Great, following the new commits they all look good to me. Were all the comments I had before addressed? If so then I didn't find any other comments since, so lgtm as a whole!

@tlively tlively merged commit 3325a9c into WebAssembly:master Dec 14, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tlively tlively deleted the tlively:minimal-literal branch Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.