Skip to content

Add IR, parsing and binary support for AtomicRMW instructions from wasm threads proposal #1082

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.

Sign up for GitHub

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

Merged
dschuff merged 10 commits into from
Jul 7, 2017
Merged

Add IR, parsing and binary support for AtomicRMW instructions from wasm threads proposal #1082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4750f31
parse atomic rmw wast
dschuff Jun 28, 2017
f79fed2
start
dschuff Jun 29, 2017
f19ccb2
add printing, fix pass traversal
dschuff Jun 30, 2017
1b58b17
add binary
dschuff Jul 5, 2017
5141e46
update tests
dschuff Jul 5, 2017
58b8727
update test, add undef
dschuff Jul 5, 2017
654fafc
reviews
dschuff Jul 6, 2017
8520d80
update tests
dschuff Jul 6, 2017
f15707d
jgravelle comments
dschuff Jul 6, 2017
c004c55
use inclusive range
dschuff Jul 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/passes/Print.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,39 @@ struct PrintSExpression : public Visitor<PrintSExpression> {
printFullLine(curr->value);
decIndent();
}
void visitAtomicRMW(AtomicRMW* curr) {
o << '(';
prepareColor(o) << printWasmType(curr->type) << ".atomic.rmw";
if (curr->bytes != getWasmTypeSize(curr->type)) {
if (curr->bytes == 1) {
o << '8';
} else if (curr->bytes == 2) {
o << "16";
} else if (curr->bytes == 4) {
o << "32";
} else {
WASM_UNREACHABLE();
}
o << "_u";
}
o << '.';
switch (curr->op) {
case Add: o << "add"; break;
case Sub: o << "sub"; break;
case And: o << "and"; break;
case Or: o << "or"; break;
case Xor: o << "xor"; break;
case Xchg: o << "xchg"; break;
}
restoreNormalColor(o);
if (curr->offset) {
o << " offset=" << curr->offset;
}
incIndent();
printFullLine(curr->ptr);
printFullLine(curr->value);
decIndent();
}
void visitConst(Const *curr) {
o << curr->value;
}
Expand Down
50 changes: 49 additions & 1 deletion src/wasm-binary.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,9 +525,55 @@ enum AtomicOpcodes {
I32AtomicStore16 = 0x1a,
I64AtomicStore8 = 0x1b,
I64AtomicStore16 = 0x1c,
I64AtomicStore32 = 0x1d
I64AtomicStore32 = 0x1d,

AtomicRMWOps_Begin = 0x1e,
I32AtomicRMWAdd = 0x1e,
I64AtomicRMWAdd = 0x1f,
I32AtomicRMWAdd8U = 0x20,
I32AtomicRMWAdd16U = 0x21,
I64AtomicRMWAdd8U = 0x22,
I64AtomicRMWAdd16U = 0x23,
I64AtomicRMWAdd32U = 0x24,
I32AtomicRMWSub = 0x25,
I64AtomicRMWSub = 0x26,
I32AtomicRMWSub8U = 0x27,
I32AtomicRMWSub16U = 0x28,
I64AtomicRMWSub8U = 0x29,
I64AtomicRMWSub16U = 0x2a,
I64AtomicRMWSub32U = 0x2b,
I32AtomicRMWAnd = 0x2c,
I64AtomicRMWAnd = 0x2d,
I32AtomicRMWAnd8U = 0x2e,
I32AtomicRMWAnd16U = 0x2f,
I64AtomicRMWAnd8U = 0x30,
I64AtomicRMWAnd16U = 0x31,
I64AtomicRMWAnd32U = 0x32,
I32AtomicRMWOr = 0x33,
I64AtomicRMWOr = 0x34,
I32AtomicRMWOr8U = 0x35,
I32AtomicRMWOr16U = 0x36,
I64AtomicRMWOr8U = 0x37,
I64AtomicRMWOr16U = 0x38,
I64AtomicRMWOr32U = 0x39,
I32AtomicRMWXor = 0x3a,
I64AtomicRMWXor = 0x3b,
I32AtomicRMWXor8U = 0x3c,
I32AtomicRMWXor16U = 0x3d,
I64AtomicRMWXor8U = 0x3e,
I64AtomicRMWXor16U = 0x3f,
I64AtomicRMWXor32U = 0x40,
I32AtomicRMWXchg = 0x41,
I64AtomicRMWXchg = 0x42,
I32AtomicRMWXchg8U = 0x43,
I32AtomicRMWXchg16U = 0x44,
I64AtomicRMWXchg8U = 0x45,
I64AtomicRMWXchg16U = 0x46,
I64AtomicRMWXchg32U = 0x47,
AtomicRMWOps_End = 0x47,
};


enum MemoryAccess {
Offset = 0x10, // bit 4
Alignment = 0x80, // bit 7
Expand Down Expand Up @@ -676,6 +722,7 @@ class WasmBinaryWriter : public Visitor<WasmBinaryWriter, void> {
void emitMemoryAccess(size_t alignment, size_t bytes, uint32_t offset);
void visitLoad(Load *curr);
void visitStore(Store *curr);
void visitAtomicRMW(AtomicRMW *curr);
void visitConst(Const *curr);
void visitUnary(Unary *curr);
void visitBinary(Binary *curr);
Expand Down Expand Up @@ -833,6 +880,7 @@ class WasmBinaryBuilder {
void readMemoryAccess(Address& alignment, size_t bytes, Address& offset);
bool maybeVisitLoad(Expression*& out, uint8_t code, bool isAtomic);
bool maybeVisitStore(Expression*& out, uint8_t code, bool isAtomic);
bool maybeVisitAtomicRMW(Expression*& out, uint8_t code);
bool maybeVisitConst(Expression*& out, uint8_t code);
bool maybeVisitUnary(Expression*& out, uint8_t code);
bool maybeVisitBinary(Expression*& out, uint8_t code);
Expand Down
1 change: 1 addition & 0 deletions src/wasm-s-parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ class SExpressionWasmBuilder {
Expression* makeConst(Element& s, WasmType type);
Expression* makeLoad(Element& s, WasmType type, bool isAtomic);
Expression* makeStore(Element& s, WasmType type, bool isAtomic);
Expression* makeAtomicRMW(Element& s, WasmType type);
Expression* makeIf(Element& s);
Expression* makeMaybeBlock(Element& s, size_t i, WasmType type);
Expression* makeLoop(Element& s);
Expand Down
10 changes: 10 additions & 0 deletions src/wasm-traversal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ struct Visitor {
ReturnType visitSetGlobal(SetGlobal* curr) {}
ReturnType visitLoad(Load* curr) {}
ReturnType visitStore(Store* curr) {}
ReturnType visitAtomicRMW(AtomicRMW* curr) {return ReturnType();} //Stub impl so not every pass has to implement this yet.
Copy link
Member

@kripken kripken Jul 6, 2017

Choose a reason for hiding this comment

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

I'm ok with landing this PR with not all opt passes working yet, we can fix those later. Could we keep this line normal-looking, then, with {} like the others? Or is the return here temporarily necessary?

Copy link
Member Author

@dschuff dschuff Jul 6, 2017

Choose a reason for hiding this comment

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

The return type is necessary because without it, non-void-returning visitors won't compile unless they have their own override. Probably all of these templates should have the return ReturnType() instead of being empty, otherwise passes can't fall back on them, which I'm guessing was not the intention.

Copy link
Member

@kripken kripken Jul 6, 2017

Choose a reason for hiding this comment

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

Right, makes sense. Yeah, I think we should update them all to the { return ReturnType; } form

ReturnType visitConst(Const* curr) {}
ReturnType visitUnary(Unary* curr) {}
ReturnType visitBinary(Binary* curr) {}
Expand Down Expand Up @@ -90,6 +91,7 @@ struct Visitor {
case Expression::Id::SetGlobalId: DELEGATE(SetGlobal);
case Expression::Id::LoadId: DELEGATE(Load);
case Expression::Id::StoreId: DELEGATE(Store);
case Expression::Id::AtomicRMWId: DELEGATE(AtomicRMW);
case Expression::Id::ConstId: DELEGATE(Const);
case Expression::Id::UnaryId: DELEGATE(Unary);
case Expression::Id::BinaryId: DELEGATE(Binary);
Expand Down Expand Up @@ -130,6 +132,7 @@ struct UnifiedExpressionVisitor : public Visitor<SubType> {
ReturnType visitSetGlobal(SetGlobal* curr) { return static_cast<SubType*>(this)->visitExpression(curr); }
ReturnType visitLoad(Load* curr) { return static_cast<SubType*>(this)->visitExpression(curr); }
ReturnType visitStore(Store* curr) { return static_cast<SubType*>(this)->visitExpression(curr); }
ReturnType visitAtomicRMW(AtomicRMW* curr) { return static_cast<SubType*>(this)->visitExpression(curr); }
ReturnType visitConst(Const* curr) { return static_cast<SubType*>(this)->visitExpression(curr); }
ReturnType visitUnary(Unary* curr) { return static_cast<SubType*>(this)->visitExpression(curr); }
ReturnType visitBinary(Binary* curr) { return static_cast<SubType*>(this)->visitExpression(curr); }
Expand Down Expand Up @@ -306,6 +309,7 @@ struct Walker : public VisitorType {
static void doVisitSetGlobal(SubType* self, Expression** currp) { self->visitSetGlobal((*currp)->cast<SetGlobal>()); }
static void doVisitLoad(SubType* self, Expression** currp) { self->visitLoad((*currp)->cast<Load>()); }
static void doVisitStore(SubType* self, Expression** currp) { self->visitStore((*currp)->cast<Store>()); }
static void doVisitAtomicRMW(SubType* self, Expression** currp) { self->visitAtomicRMW((*currp)->cast<AtomicRMW>()); }
static void doVisitConst(SubType* self, Expression** currp) { self->visitConst((*currp)->cast<Const>()); }
static void doVisitUnary(SubType* self, Expression** currp) { self->visitUnary((*currp)->cast<Unary>()); }
static void doVisitBinary(SubType* self, Expression** currp) { self->visitBinary((*currp)->cast<Binary>()); }
Expand Down Expand Up @@ -428,6 +432,12 @@ struct PostWalker : public Walker<SubType, VisitorType> {
self->pushTask(SubType::scan, &curr->cast<Store>()->ptr);
break;
}
case Expression::Id::AtomicRMWId: {
self->pushTask(SubType::doVisitAtomicRMW, currp);
self->pushTask(SubType::scan, &curr->cast<AtomicRMW>()->value);
self->pushTask(SubType::scan, &curr->cast<AtomicRMW>()->ptr);
break;
}
case Expression::Id::ConstId: {
self->pushTask(SubType::doVisitConst, currp);
break;
Expand Down
31 changes: 24 additions & 7 deletions src/wasm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,10 @@ enum HostOp {
PageSize, CurrentMemory, GrowMemory, HasFeature
};

enum AtomicRMWOp {
Add, Sub, And, Or, Xor, Xchg,
};

//
// Expressions
//
Expand Down Expand Up @@ -177,6 +181,7 @@ class Expression {
HostId,
NopId,
UnreachableId,
AtomicCmpxchgId,
AtomicRMWId,
NumExpressionIds
};
Expand Down Expand Up @@ -423,6 +428,25 @@ class Store : public SpecificExpression<Expression::StoreId> {
void finalize();
};

class AtomicRMW : public SpecificExpression<Expression::AtomicRMWId> {
public:
AtomicRMW() = default;
AtomicRMW(MixedArena& allocator) : AtomicRMW() {}

AtomicRMWOp op;
uint8_t bytes;
Address offset;
Expression* ptr;
Expression* value;

void finalize();
};

class AtomicCmpxchg : public SpecificExpression<Expression::AtomicCmpxchgId> {
public:
AtomicCmpxchg() = default;
};

class Const : public SpecificExpression<Expression::ConstId> {
public:
Const() {}
Expand Down Expand Up @@ -514,13 +538,6 @@ class Unreachable : public SpecificExpression<Expression::UnreachableId> {
Unreachable(MixedArena& allocator) : Unreachable() {}
};

class AtomicRMW : public SpecificExpression<Expression::AtomicRMWId> {
public:
AtomicRMW() {}
AtomicRMW(MixedArena& allocator) : AtomicRMW() {}
bool finalize();
};

// Globals

class Function {
Expand Down
90 changes: 90 additions & 0 deletions src/wasm/wasm-binary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -832,6 +832,51 @@ void WasmBinaryWriter::visitStore(Store *curr) {
emitMemoryAccess(curr->align, curr->bytes, curr->offset);
}

void WasmBinaryWriter::visitAtomicRMW(AtomicRMW *curr) {
if (debug) std::cerr << "zz node: AtomicRMW" << std::endl;
recurse(curr->ptr);
recurse(curr->value);

o << int8_t(BinaryConsts::AtomicPrefix);

#define CASE_FOR_OP(Op) \
case Op: \
switch (curr->type) { \
case i32: \
switch (curr->bytes) { \
case 1: o << int8_t(BinaryConsts::I32AtomicRMW##Op##8U); break; \
case 2: o << int8_t(BinaryConsts::I32AtomicRMW##Op##16U); break; \
case 4: o << int8_t(BinaryConsts::I32AtomicRMW##Op); break; \
default: WASM_UNREACHABLE(); \
} \
break; \
case i64: \
switch (curr->bytes) { \
case 1: o << int8_t(BinaryConsts::I64AtomicRMW##Op##8U); break; \
case 2: o << int8_t(BinaryConsts::I64AtomicRMW##Op##16U); break; \
case 4: o << int8_t(BinaryConsts::I64AtomicRMW##Op##32U); break; \
case 8: o << int8_t(BinaryConsts::I64AtomicRMW##Op); break; \
default: WASM_UNREACHABLE(); \
} \
break; \
default: WASM_UNREACHABLE(); \
} \
break

switch(curr->op) {
CASE_FOR_OP(Add);
CASE_FOR_OP(Sub);
CASE_FOR_OP(And);
CASE_FOR_OP(Or);
CASE_FOR_OP(Xor);
CASE_FOR_OP(Xchg);
default: WASM_UNREACHABLE();
}
#undef CASE_FOR_OP
Copy link
Contributor

@jgravelle-google jgravelle-google Jul 6, 2017

Choose a reason for hiding this comment

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

Yay for big ugly macros that make the actual code easier to read.
And yay for cleaning up afterward.


emitMemoryAccess(curr->bytes, curr->bytes, curr->offset);
}

void WasmBinaryWriter::visitConst(Const *curr) {
if (debug) std::cerr << "zz node: Const" << curr << " : " << curr->type << std::endl;
switch (curr->type) {
Expand Down Expand Up @@ -1934,6 +1979,7 @@ BinaryConsts::ASTNodes WasmBinaryBuilder::readExpression(Expression*& curr) {
code = getInt8();
if (maybeVisitLoad(curr, code, /*isAtomic=*/true)) break;
if (maybeVisitStore(curr, code, /*isAtomic=*/true)) break;
if (maybeVisitAtomicRMW(curr, code)) break;
throw ParseException("invalid code after atomic prefix: " + std::to_string(code));
}
default: {
Expand Down Expand Up @@ -2282,6 +2328,50 @@ bool WasmBinaryBuilder::maybeVisitStore(Expression*& out, uint8_t code, bool isA
return true;
}


bool WasmBinaryBuilder::maybeVisitAtomicRMW(Expression*& out, uint8_t code) {
if (code < BinaryConsts::AtomicRMWOps_Begin || code > BinaryConsts::AtomicRMWOps_End) return false;
auto* curr = allocator.alloc<AtomicRMW>();

// Set curr to the given opcode, type and size.
#define SET(opcode, optype, size) \
curr->op = opcode; \
curr->type = optype; \
curr->bytes = size
Copy link
Contributor

@jgravelle-google jgravelle-google Jul 6, 2017

Choose a reason for hiding this comment

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

Not sure if I should complain that a missing terminal semicolon is a bit odd looking, considering that this is a nested macro that's right next to its use.

I guess: at a quick skim, this gave me a double take, but this is almost-certainly OK.

Copy link
Member Author

@dschuff dschuff Jul 6, 2017

Choose a reason for hiding this comment

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

Yeah, most function-like macros lack the semicolon so you can do SET(a, b, c); but this (and especially the case-based ones) are a little funny. To fix SET I could wrap them in the standard do {} while(0) but not sure that would be an improvement.

Copy link
Member

@binji binji Jul 6, 2017

Choose a reason for hiding this comment

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

I often add a comment when I do this in a macro, e.g.:

/* no semicolon */

Kind of like adding // fallthrough when falling throw a case statement.


// Handle the cases for all the valid types for a particular opcode
#define SET_FOR_OP(Op) \
case BinaryConsts::I32AtomicRMW##Op: SET(Op, i32, 4); break; \
case BinaryConsts::I32AtomicRMW##Op##8U: SET(Op, i32, 1); break; \
case BinaryConsts::I32AtomicRMW##Op##16U: SET(Op, i32, 2); break; \
case BinaryConsts::I64AtomicRMW##Op: SET(Op, i64, 8); break; \
case BinaryConsts::I64AtomicRMW##Op##8U: SET(Op, i64, 1); break; \
case BinaryConsts::I64AtomicRMW##Op##16U: SET(Op, i64, 2); break; \
case BinaryConsts::I64AtomicRMW##Op##32U: SET(Op, i64, 4); break;

switch(code) {
SET_FOR_OP(Add);
SET_FOR_OP(Sub);
SET_FOR_OP(And);
SET_FOR_OP(Or);
SET_FOR_OP(Xor);
SET_FOR_OP(Xchg);
default: WASM_UNREACHABLE();
}
#undef SET_FOR_OP
#undef SET

if (debug) std::cerr << "zz node: AtomicRMW" << std::endl;
Address readAlign;
readMemoryAccess(readAlign, curr->bytes, curr->offset);
if (readAlign != curr->bytes) throw ParseException("Align of AtomicRMW must match size");
curr->value = popNonVoidExpression();
curr->ptr = popNonVoidExpression();
curr->finalize();
out = curr;
return true;
}

bool WasmBinaryBuilder::maybeVisitConst(Expression*& out, uint8_t code) {
Const* curr;
if (debug) std::cerr << "zz node: Const, code " << code << std::endl;
Expand Down
Loading