[PATCH] D15197: [WebAssembly] Support constant offsets on loads and stores

[Dan Gohman via llvm-commits]

sunfish added a comment.

This looks like a good direction. I like how it uses def : Pat instead of needing custom code in WebAssemblyISelDAGToDAG.cpp :-).

One tricky thing though is that wasm's constant offsets are unsigned, so we can't support negative values. I think we'll need to add a predicate on the pattern-match which checks this. I think it's fine if you'd prefer to just add a FIXME for this for now and we can address it later though. Or if you want to simplify things, we could also defer pattern-matching for constant offsets altogether for now, since framepointer lowering doesn't actually require it.


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrMemory.td:106
@@ +105,3 @@
+def : Pat<(store I32:$val, (add I32:$addr, (i32 imm:$off))),
+          (STORE_I32 imm:$off, I32:$addr, I32:$val)>;
+
----------------
dschuff wrote:
> writing the offset operand as 'imm:$off' turns out to be critical here to getting an immediate in the output. If you say just $off or I32:$off you still get a pattern match but it generates an i32.const and uses that instead of putting it directly as an immediate in the store instruction. (The same applies if you use this kind of pattern to match a load, but if you put it directly in the definition for the load as I did here, then it doesn't matter if you use the imm: prefix or not). I'm not sure why that is.
If you say I32:$off, it refers to the I32 register class (defined in lib/Target/WebAssembly/WebAssemblyRegisterInfo.td), which effectively says that you need the value in a register of that class, which forces the instruction selector to use an i32.const to get the value into a register. I don't know the rules if you just say $off by itself. I'd suggest using the imm: prefix to avoid trouble :-).


http://reviews.llvm.org/D15197