This is an archive of the discontinued LLVM Phabricator instance.

WebAssembly: basic instructions todo, and basic register info.
ClosedPublic

Authored by jfb on Jul 9 2015, 10:55 AM.

Details

Summary

This code is based on AArch64 for modern backend good practice, and NVPTX for
virtual ISA concerns.

Diff Detail

Repository
rL LLVM

Event Timeline

jfb updated this revision to Diff 29358.Jul 9 2015, 10:55 AM
jfb retitled this revision from to WebAssembly: basic instructions todo, and basic register info..
jfb added a reviewer: sunfish.
jfb updated this object.
jfb added a subscriber: llvm-commits.
jfb added a comment.Jul 9 2015, 11:10 AM

I'll do a follow-up PR to address the FIXME on needsStackRealignment.

sunfish requested changes to this revision.Jul 9 2015, 1:42 PM
sunfish edited edge metadata.

Cool. Here's a first round of comments.

lib/Target/WebAssembly/CMakeLists.txt
3 ↗(On Diff #29358)

Please keep these rules alphabetized.

lib/Target/WebAssembly/Makefile
16 ↗(On Diff #29358)

Common practice in these Makefiles is to indent subsequent lines to be to the right of the '='.

lib/Target/WebAssembly/WebAssemblyInstrAtomics.td
12 ↗(On Diff #29358)

For full doxygen happiness, these should get triple-slash comments, as is done in other files.

lib/Target/WebAssembly/WebAssemblyInstrCall.td
1 ↗(On Diff #29358)

If we have extra characters available in the top-line comment, it's nice to leave in some of the '='s that LLVM typically uses at the beginning and end.

lib/Target/WebAssembly/WebAssemblyInstrConv.td
1 ↗(On Diff #29358)

"Conv" in closely-related contexts is short for Convention, as in CallingConv. I'm ok with the filename here, but can you make the top-level comment clarify that this is about conversions?

lib/Target/WebAssembly/WebAssemblyInstrInfo.td
40 ↗(On Diff #29358)

Is there a reason for reordering these? Operands and DAG Nodes and DAG Node Types are all inter-related so it's nice to have them together.

50 ↗(On Diff #29358)

If comma and conditional end up being included in WebAssembly, they'll have embedded control flow, so we won't be able to represent them explicitly in either SelectionDAG or MachineInstrs.

lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp
49 ↗(On Diff #29358)

This should also set FP32/FP64 as reserved, at least when TFI->hasFP(), but maybe always, since these are virtual registers anyway.

56 ↗(On Diff #29358)

report_fatal_error is for legitimate conditions that codegen can't handle, not for reporting bugs in codegen. llvm_unreachable is more appropriate here because it's just a "bug" that this function isn't implemented yet.

61 ↗(On Diff #29358)

Using unsigned instead of unsigned char for holding register values.

65 ↗(On Diff #29358)

We should static_cast this to WebAssemblyFrameLowering so that the hasFP call can be easily devirtualized.

lib/Target/WebAssembly/WebAssemblyRegisterInfo.td
13 ↗(On Diff #29358)

More triple-slash comments.

30 ↗(On Diff #29358)

WebAssembly won't support it right away, and it's not yet decided whether it will support it in the future. But actually, we need separate registers anyway so that we can put them in separate register classes.

43 ↗(On Diff #29358)

This code comes from NVPTXRegisterInfo.td, and we are doing something similar here, but I'm curious what it's really needed for. Could we try leaving it out and seeing what breaks?

This revision now requires changes to proceed.Jul 9 2015, 1:42 PM
jfb updated this revision to Diff 29404.Jul 9 2015, 3:47 PM
jfb edited edge metadata.

Rebase.

jfb updated this revision to Diff 29409.Jul 9 2015, 4:17 PM
jfb marked 13 inline comments as done.
jfb edited edge metadata.
  • Address sunfish's comments.
jfb added a comment.Jul 9 2015, 4:17 PM

Updated, PTAL.

lib/Target/WebAssembly/Makefile
16 ↗(On Diff #29404)

Those are tabs (same as for other Makefiles), they just don't appear nicely in phab.

lib/Target/WebAssembly/WebAssemblyInstrInfo.td
50 ↗(On Diff #29404)

Good point, I removed them.

lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp
49 ↗(On Diff #29404)

Oops, I added FP later and forgot to add it here too. Changed to a range-based for loop too.

61 ↗(On Diff #29404)

Imagine all the bits we're wasting! ;-)

65 ↗(On Diff #29404)

I'll send a separate PR to do the same in other backends.

lib/Target/WebAssembly/WebAssemblyRegisterInfo.td
30 ↗(On Diff #29404)

Are you agreeing, or suggesting something?

43 ↗(On Diff #29404)

I'll add a todo instead to try it out later, if needed? I'd rather follow what others have done, and then try to change it, seems like we'll have less WTF moments that way.

sunfish accepted this revision.Jul 10 2015, 11:10 AM
sunfish edited edge metadata.

Looks good.

lib/Target/WebAssembly/WebAssemblyRegisterInfo.td
30 ↗(On Diff #29409)

I think I'm just suggesting the comment say something like "may someday support" instead of "supports".

This revision is now accepted and ready to land.Jul 10 2015, 11:10 AM
jfb updated this revision to Diff 29462.Jul 10 2015, 11:21 AM
jfb edited edge metadata.
  • Address sunfish's comments.
  • Use the shiny new devirtualized getFrameLowering from D11093.
  • 'May someday support'.
jfb marked an inline comment as done.Jul 10 2015, 11:21 AM

Clarified the comment, and updated the patch to use devirtualized frame lowering.

This revision was automatically updated to reflect the committed changes.