This is an archive of the discontinued LLVM Phabricator instance.

WebAssembly: generate load/store
ClosedPublic

Authored by jfb on Aug 28 2015, 6:05 PM.

Details

Reviewers
sunfish
Summary

This handles all load/store operations that WebAssembly defines, and handles those necessary for C++ such as i1. I left a FIXME for outstanding features which aren't required for now.

Diff Detail

Event Timeline

jfb updated this revision to Diff 33503.Aug 28 2015, 6:05 PM
jfb retitled this revision from to WebAssembly: generate load/store.
jfb updated this object.
jfb added a reviewer: sunfish.
jfb added a subscriber: llvm-commits.
sunfish accepted this revision.Aug 31 2015, 12:08 PM
sunfish edited edge metadata.
sunfish added inline comments.
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
94

Is there a reason why OpcodeName has to be a member function?

103

Ditto?

103

And ditto here? Having this be a standalone function nicely emphasizes that it's just a pure function from APFloat to string.

lib/Target/WebAssembly/WebAssemblyInstrMemory.td
33

Nit: Why the trailing underscore in the name?

62

I'm ok with expanding things out like this, but several other things in the WebAssembly tablegen files are aggressively factored out into multiclasses. Is there a reason these aren't multiclasses too?

71

Please add a comment mentioning that WebAssembly's operand order is the opposite of SelectionDAG's operand order here.

This revision is now accepted and ready to land.Aug 31 2015, 12:08 PM
jfb updated this revision to Diff 33632.Aug 31 2015, 2:26 PM
jfb marked 5 inline comments as done.
jfb edited edge metadata.
  • AsmPrinter: back to static functions.
  • Note on store operand order.
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
94

Name lookup was sad because I have two toString methods, but I changed the Type version to take in hasAddr64 and moved everything back to static, so it's all good.

lib/Target/WebAssembly/WebAssemblyInstrMemory.td
33

I'm playing tricks with the name printing: it truncates out the last underscore and everything after it, so this prints as load_i32. We can come up with another scheme (maybe pass in a string), but this was pretty simple to start with.

62

Maybe I'm lacking imagination: InstrFormats-style multiclasses would only be used once each here? There's one load per type, and the #_I32 concatenation has to be textual IIUC. Similarly for extending loads? Or maybe tablegen has extra tricks I don't know :-)

I was thinking that HasAddr64 should be done with a multiclass for Ptr:$addr and require, but I figure this should be a separate patch.

jfb closed this revision.Sep 1 2015, 8:55 AM

Committed in reviews.llvm.org/rL246500.