Add IR, parsing and binary support for AtomicRMW instructions from wasm threads proposal by dschuff · Pull Request #1082 · WebAssembly/binaryen

dschuff · 2017-07-06T00:03:57Z

Also leave a stub (but valid) visitAtomicRMW in the visitor template so that not all visitors need to implement this function yet.

dschuff · 2017-07-06T00:04:25Z

Future PRs will add the visitors, including validation, optimizations, etc.

kripken

We should update EffectAnalyzer in ast_utils.h, these new opts probably need to set readsMemory and writesMemory, possibly also implicitTrap. (can leave for followups too)

kripken · 2017-07-06T00:17:43Z

src/wasm-traversal.h

  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.


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?

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.

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

kripken · 2017-07-06T00:20:27Z

src/wasm/wasm.cpp

 }

+void AtomicRMW::finalize() {
+  if (ptr->type == unreachable || value->type == unreachable)


{, } around if body

kripken · 2017-07-06T00:21:20Z

test/atomics.wast

 (type $0 (func))
 (memory $0 23 256 shared)
- (func $atomics (type $0)
+ (func $atomic-loadstoare (type $0)


Looks like a typo in store here, must be in the Print code?

This is the original handwritten test file, so it was just an old-fashioned typo in my fingers :)

kripken · 2017-07-06T16:36:48Z

lgtm with that.

jgravelle-google · 2017-07-06T16:21:54Z

src/wasm/wasm-binary.cpp

+    CASE_FOR_OP(Xchg);
+    default: WASM_UNREACHABLE();
+  }
+#undef CASE_FOR_OP


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

jgravelle-google · 2017-07-06T16:27:36Z

src/wasm/wasm-binary.cpp


+
+bool WasmBinaryBuilder::maybeVisitAtomicRMW(Expression*& out, uint8_t code) {
+  if (code < BinaryConsts::I32AtomicRMWAdd || code > BinaryConsts::I64AtomicRMWXchg32U) return false;


These bounds look arbitrary without referencing BinaryConsts' definition (and/or the threads proposal).

Would it be worth adding at the end of BinaryConsts' definition something like

AtomicRMW_BEGIN = I32AtomicRMWAdd, AtomicRMW_END = I64AtomicRMWXchg32U,

jgravelle-google · 2017-07-06T16:30:44Z

src/wasm/wasm-binary.cpp

+#define SET(opcode, optype, size) \
+  curr->op = opcode;              \
+  curr->type = optype;            \
+  curr->bytes = size


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.

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.

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.

jgravelle-google · 2017-07-06T16:43:37Z

src/wasm/wasm-s-parser.cpp

+  return ret;
+}
+
+static size_t parseMemAttributes(Element& s, Address* offset, Address* align, Address fallback) {


Yay method extraction

jgravelle-google · 2017-07-06T16:49:31Z

src/wasm/wasm-s-parser.cpp

-      uint64_t align = atoll(eq);
-      if (align > std::numeric_limits<uint32_t>::max()) throw ParseException("bad align");
-      ret->align = align;
+      uint64_t a = atoll(eq);


a and o are kinda iffy names, but they're very local and are probably fine. But they have the same initialization code and so this might be more clear as:

uint64_t value = atoll(eq); if (value > std::numeric_limits<uint32_t>::max()) throw ParseException("memory attribute value too large"); if (str[0] == 'a') { *align = value; } else if (str[0] == 'o') { *offset = value; } else throw ParseException("bad memory attribute");

Yeah I like that better.

dschuff · 2017-07-06T17:13:38Z

I figured "end" was consistent with C++ iterator ends and other general c-style bounds where the end is one-past-the-end.

dschuff · 2017-07-06T17:14:35Z

FWIW I changed it because the bounds check as it was written before looked funny to me :)

jgravelle-google · 2017-07-06T17:15:47Z

Ah, yeah makes sense.

dschuff added 6 commits July 5, 2017 16:50

parse atomic rmw wast

4750f31

start

f79fed2

add printing, fix pass traversal

f19ccb2

add binary

1b58b17

update tests

5141e46

update test, add undef

58b8727

kripken reviewed Jul 6, 2017

View reviewed changes

dschuff added 2 commits July 6, 2017 09:28

reviews

654fafc

update tests

8520d80

jgravelle-google reviewed Jul 6, 2017

View reviewed changes

jgravelle comments

f15707d

jgravelle-google approved these changes Jul 6, 2017

View reviewed changes

use inclusive range

c004c55

dschuff merged commit 4995132 into master Jul 7, 2017

dschuff mentioned this pull request Jul 10, 2017

Wasm threading/shared memory support #1085

Closed

10 tasks

dschuff deleted the atomicrmw branch July 10, 2017 21:08



		bool WasmBinaryBuilder::maybeVisitAtomicRMW(Expression*& out, uint8_t code) {
		if (code < BinaryConsts::I32AtomicRMWAdd \|\| code > BinaryConsts::I64AtomicRMWXchg32U) return false;

Conversation

dschuff commented Jul 6, 2017

Uh oh!

dschuff commented Jul 6, 2017

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Jul 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dschuff commented Jul 6, 2017

Uh oh!

dschuff commented Jul 6, 2017

Uh oh!

jgravelle-google commented Jul 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants