Refine alignment #31
Refine alignment #31
Conversation
| @@ -241,6 +239,13 @@ and check_arm c t ts arm = | |||
| check_literal c [t] l; | |||
| check_expr c (if fallthru then [] else ts) e | |||
|
|
|||
| and check_memop memop at = | |||
| require (memop.align = 1 || memop.align = 2 || | |||
| memop.align = 4 || memop.align = 8) | |||
There was a problem hiding this comment.
Since we're checking that the alignment is at most the access size below, would it make sense to just check that the alignment is a power of 2 here, instead of having an explicit list? That way it'll automatically generalize to SIMD types.
There was a problem hiding this comment.
Agreed.
Extra advantage: subsequent indexed memory accesses can also key off the first access' alignment (base has an alignment, other indices don't because the alignment is implied).
|
Lgtm, other than README ;) |
lukewagner
force-pushed
the
refine-alignment
branch
from
a2021bf to
d390178
Compare
lukewagner
force-pushed
the
refine-alignment
branch
from
d390178 to
cd22c83
Compare
|
Gosh, the README, it is my blind spot. lmk if |
| @@ -26,6 +27,17 @@ | |||
| (module (memory 100 1000 (segment 0 "a") (segment 2 "b") (segment 1 "c"))) | |||
| "data segment not disjoint and ordered") | |||
|
|
|||
| ;; Test alignment annotation rules | |||
| (assertinvalid (module (func (loadu.2.i8 (const.i32 0)))) "alignment too big") | |||
There was a problem hiding this comment.
Too big alignment is part of the design so this test isn't correct.
There was a problem hiding this comment.
The design document isn't expected to be a complete enumeration of all invalid conditions, that's what spec is for and so this test is correct, wrt the spec as updated by this PR. If you would like to disagree and argue that we should allow too-big alignment in the spec, that's another matter entirely, we shouldn't expect the high-level design repo to have to reiterate every single minute detail; that's not its purpose.
There was a problem hiding this comment.
Agreed that the design doc isn't expected to be complete, but this specific case was discussed when creating the design, and we agreed that alignment just had to be a positive power of 2, no limits. This spec PR contradicts the design and what we agreed on, so the design is now wrong.
In cases like this the discussion should occur in the design repo: it revisits a decision that was made in the design repo. What the point of agreeing on anything in the design repo otherwise? I'm not saying that the design is golden and shouldn't change (witness the issues I've filed recently), I'm saying that we want to avoid diverging slightly in every place we implement wasm.
It's great the the spec repo finds unforeseen problems with the design, but this one wasn't unforeseen!
There was a problem hiding this comment.
I was not aware that anyone had discussed this issue; could you provide a link? This seems like the sort of decision that should be explicitly documented since it's nonobvious whether "power of 2" is just waving its hands at the boundary conditions (as we often do) or really means "no, seriously, every power of 2!" (up to what limit?). I'm happy to remove this check here for now since ultimately it's a minor detail and people have actually discussed it.
There was a problem hiding this comment.
Anyhow, now it's explicit in code and a test; the process is working!
|
Merging based on lgtm above and no outstanding concerns. |
* Add clearer examples to Globals.md for rationale * Fix destructuring * Add comment about using globals for other TLS values * Add example using WebAssembly.Global
…mory.init`, `memory.drop`, `table.init`, and `table.drop` identify passive segments; only that they index a valid segment. (WebAssembly#31)
…statement Fix reference to `bind` statement
* Add a simple and more involved generator example * More examples
Merge with WebAssembly/spec
Extending #30, this patch moves the alignment aspect of linear memory ops to what's in the design repo as follows:
Memory.alignmentfromMemorysince alignment makes no semantic differenceAst.alignment, since load/store AST nodes do have an alignment fieldalignmentfrom a binaryAligned|Unalignedto anintwhich is validated to be {1,2,4,8} and smaller than the element size