[PATCH] D11070: WebAssembly: basic instructions todo, and basic register info.

[Dan Gohman]

sunfish requested changes to this revision.
sunfish added a comment.
This revision now requires changes to proceed.

Cool. Here's a first round of comments.


================
Comment at: lib/Target/WebAssembly/CMakeLists.txt:3
@@ -2,2 +2,3 @@
 
+tablegen(LLVM WebAssemblyGenRegisterInfo.inc -gen-register-info)
 tablegen(LLVM WebAssemblyGenMCCodeEmitter.inc -gen-emitter)
----------------
Please keep these rules alphabetized.

================
Comment at: lib/Target/WebAssembly/Makefile:16
@@ -16,1 +15,3 @@
+BUILT_SOURCES = WebAssemblyGenRegisterInfo.inc WebAssemblyGenSubtargetInfo.inc \
+		WebAssemblyGenMCCodeEmitter.inc
 
----------------
Common practice in these Makefiles is to indent subsequent lines to be to the right of the '='.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrAtomics.td:12
@@ -11,2 +11,3 @@
+// \brief WebAssembly Atomic operand code-gen constructs.
 //
 //===----------------------------------------------------------------------===//
----------------
For full doxygen happiness, these should get triple-slash comments, as is done in other files.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrCall.td:1
@@ -1,2 +1,2 @@
-// WebAssemblyInstrSIMD.td - WebAssembly SIMD codegen support -*- tablegen -*-//
+// WebAssemblyInstrCall.td-WebAssembly Call codegen support -----*- tablegen -*-
 //
----------------
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.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrConv.td:1
@@ +1,2 @@
+// WebAssemblyInstrConv.td-WebAssembly Conv codegen support -----*- tablegen -*-
+//
----------------
"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?

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrInfo.td:40
@@ -37,2 +39,3 @@
+// WebAssembly-specific Operands.
 //===----------------------------------------------------------------------===//
 
----------------
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.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrInfo.td:50
@@ +49,3 @@
+ *        return the second operand
+ * conditional: basically ternary ?: operator
+*/
----------------
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.

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp:49
@@ +48,3 @@
+  Reserved.set(WebAssembly::SP32);
+  Reserved.set(WebAssembly::SP64);
+  return Reserved;
----------------
This should also set FP32/FP64 as reserved, at least when TFI->hasFP(), but maybe always, since these are virtual registers anyway.

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp:56
@@ +55,3 @@
+    RegScavenger *RS) const {
+  report_fatal_error("WebAssemblyRegisterInfo::eliminateFrameIndex"); // FIXME
+}
----------------
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.

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp:61
@@ +60,3 @@
+WebAssemblyRegisterInfo::getFrameRegister(const MachineFunction &MF) const {
+  static const unsigned char Regs[2][2] = {
+      /*            !isArch64Bit       isArch64Bit      */
----------------
Using `unsigned` instead of `unsigned char` for holding register values.

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp:65
@@ +64,3 @@
+      /*  hasFP */ {WebAssembly::FP32, WebAssembly::FP64}};
+  const TargetFrameLowering *TFI = MF.getSubtarget().getFrameLowering();
+  return Regs[TFI->hasFP(MF)][TT.isArch64Bit()];
----------------
We should static_cast this to WebAssemblyFrameLowering so that the hasFP call can be easily devirtualized.

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegisterInfo.td:13
@@ -11,3 +12,3 @@
 // physical registers.
 //
 //===----------------------------------------------------------------------===//
----------------
More triple-slash comments.

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegisterInfo.td:30
@@ +29,3 @@
+// WebAssembly supports mixed 32-bit and 64-bit heaps in the same application,
+// which requires separate width FP and SP.
+def FP32 : WebAssemblyReg<"%FP32">;
----------------
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.

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegisterInfo.td:43
@@ +42,3 @@
+  def D#i : WebAssemblyReg<"%d."#i>; // f64
+}
+
----------------
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?


http://reviews.llvm.org/D11070