This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Implement unaligned loads and stores.
ClosedPublic

Authored by sunfish on Jan 25 2016, 8:00 AM.

Details

Summary

WebAssembly loads and stores have an alignment attribute, so they support any alignment efficiently, provided that the alignment is correctly declared. This patch adds support for correctly declaring alignment values.

Diff Detail

Repository
rL LLVM

Event Timeline

sunfish updated this revision to Diff 45867.Jan 25 2016, 8:00 AM
sunfish retitled this revision from to [WebAssembly] Implement unaligned loads and stores..
sunfish updated this object.
sunfish set the repository for this revision to rL LLVM.
jfb added inline comments.Jan 25 2016, 4:40 PM
lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp
180

Could you name the method differently do it reflects the power-of-two-byte unit?

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
50

OPERAND_P2ALIGNMENT?

lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp
87

Can you line-comment that this is p2align? I find the implicit contracts in MI to be hard to understand when reading the code years after it was written :-)

lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
294

Should this have:

if (Fast)
  *Fast = false;

To discourage that code generation?

lib/Target/WebAssembly/WebAssemblyISelLowering.h
61

Can you drop the defaults? The override doesn't need them I think.

lib/Target/WebAssembly/WebAssemblyPeephole.cpp
74

Could you add symbolic constants for the operand numbers? 3 -> 4 is pretty obscure :-)

lib/Target/WebAssembly/WebAssemblySetAlignments.cpp
11

Sad that you need this pass at all.

This revision was automatically updated to reflect the committed changes.
sunfish marked 6 inline comments as done.Jan 25 2016, 7:46 PM
sunfish added inline comments.
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
294

Actually, I believe we want to encourage that code generation. On hardware that doesn't support fast unaligned accesses, engines should compile i64.store align=4 into two stores anyway, so it's better for the wasm to have that than two explicit stores. I added a comment for this.

jfb added a comment.Jan 25 2016, 10:29 PM

lgtm after typo fix. Very nice :)

llvm/trunk/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
296 ↗(On Diff #45944)

Ah yes, you're entirely right on this :-)

Typo: adjacent.