[PATCH] [WebAssembly] Skeleton WebAssembly target

Dan Gohman dan433584 at gmail.com
Mon Jun 29 16:01:28 PDT 2015 

================
Comment at: CODE_OWNERS.TXT:70
@@ +69,3 @@
+E: sunfish at mozilla.com
+D: WebAssembly Backend (lib/Target/WebAssembly/*)
+
----------------
jfb wrote:
> Could you add me as well for WebAssembly? I could do a different change too...
There doesn't seem to be a precedent for having multiple code owners. I think this is something we should talk with others in the LLVM community about first. Fortunately, one doesn't need to be a code owner to review patches, so I don't think we need to block on this.

================
Comment at: include/llvm/ADT/Triple.h:89
@@ -88,1 +88,3 @@
+    wasm32,     // WebAssembly with 32-bit pointers
+    wasm64,     // WebAssembly with 64-bit pointers
     LastArchType = shave
----------------
jfb wrote:
> Add a mention of little-endian in both comment.
Other targets that don't have a big-endian variant don't do this.

================
Comment at: include/llvm/ADT/Triple.h:156
@@ -153,2 +155,3 @@
     PS4,
-    LastOSType = PS4
+    WebAssembly,
+    LastOSType = WebAssembly
----------------
jfb wrote:
> I'd rather spell the OS enum the same as its textual representation in IR (uppercase enum, lowercase string seems to be the trend). That's not a problem anymore since arch are `wasm32`/`wasm64`, so I'd rather have `WASM` as the OS.
This is following the convention of using WebAssembly in capital-letters contexts, and wasm in lower-letters contexts.

================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp:35
@@ +34,3 @@
+                                          unsigned RegNo) const {
+  llvm_unreachable("TODO: implement printRegName");
+}
----------------
jfb wrote:
> `report_fatal_error` instead, here and below?
This code is totally unusable without this function, so it doesn't really matter. Crashing is better because we get a stack trace to where we need to write code.

================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.cpp:24
@@ +23,3 @@
+
+WebAssemblyMCAsmInfo::WebAssemblyMCAsmInfo() {
+  AlignmentIsInBytes = false;
----------------
jfb wrote:
> `PointerSize` is 4 by default, we need to set it for `wasm64`.
> `CalleeSaveStackSlotSize` doesn't seem relevant, so the 4 default is fine?
> 
> (a bunch of others have OK defaults)
> 
> MaxInstLength doesn't seem relevant? Default is 4, but we don't have inline asm that matters because we're doing an AST. Maybe comment?
> Same for MinInstAlignment.
> 
> (a bunch of others have OK defaults)
> 
> Should we do anything about `Code16Directive` / `Code32Directive` / `Code64Directive`? They don't make sense for us, so default is OK.
> 
> (a bunch of others have OK defaults)
> 
> What about `UseIntegratedAssembler`?
Ok, I set PointerSize and CalleeSaveStackSlotSize. I added a comment for MaxInstLength. MinInstAlignment seems fine with the default for now. I think we're ok with the defaults for the .code directives (which we probably won't use). I added a comment for UseIntegratedAssembler.

================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.cpp:25
@@ +24,3 @@
+WebAssemblyMCAsmInfo::WebAssemblyMCAsmInfo() {
+  AlignmentIsInBytes = false;
+  COMMDirectiveAlignmentIsInBytes = false;
----------------
jfb wrote:
> It seems better to have `.align` be bytes? The historical confusion is annoying, so I'd just force people to use `.balign` and `.p2align` anyways, but I think in general folks expect `.align` to be bytes?
I'm all for forcing people to use .p2align/.balign; is that an option? Otherwise, looking at LLVM's other targets, making .align be .p2align seems fairly popular.

================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.cpp:29
@@ +28,3 @@
+  SupportsDebugInformation = true;
+  UseDataRegionDirectives = true;
+
----------------
jfb wrote:
> Move this up, so it's in the same order as declared in the struct.
Ok, sorted.

================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.cpp:35
@@ +34,3 @@
+  PrivateGlobalPrefix = ".L";
+  PrivateLabelPrefix = ".L";
+  HasDotTypeDotSizeDirective = false;
----------------
jfb wrote:
> The defaults are `"L"`, which is the dot needed?
"L" as a symbol prefix is an ugly name mangling because it requires all C symbols be mangled too (since 'L' is a valid C identifier character). I've now changed this to be "", and we can plan to use directives to set symbol visibilities rather than prefixes.

================
Comment at: lib/Target/WebAssembly/TargetInfo/WebAssemblyTargetInfo.cpp:28
@@ +27,3 @@
+  RegisterTarget<Triple::wasm64> Y(TheWebAssemblyTarget, "wasm64",
+                                   "WebAssembly 64-bit");
+}
----------------
jfb wrote:
> add "(little endian)" to the description.
Other targets that don't have big-endian variants don't do this.

================
Comment at: lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp:42
@@ +41,3 @@
+bool WebAssemblyFrameLowering::hasFP(const MachineFunction &MF) const {
+  llvm_unreachable("TODO: implement hasFP");
+}
----------------
jfb wrote:
> `return_fatal_error` in this file too.
As above. Nothing works if this doesn't work.

================
Comment at: lib/Target/WebAssembly/WebAssemblyFrameLowering.h:10
@@ +9,3 @@
+///
+/// \file
+/// \brief This class implements WebAssembly-specific bits of
----------------
jfb wrote:
> CodingStandards.rst does say that `\file` and `\brief` should be in every file header. If we're trying to be on top of the standard then other files should also have this (it looks like you followed the local convention per-file, instead of blindly doing all-doxygen).
I missed adding \file and \brief in InstPrinter/*. Fixed now.

================
Comment at: lib/Target/WebAssembly/WebAssemblyFrameLowering.h:30
@@ +29,3 @@
+                            /*StackRealignable=*/true) {}
+
+  void
----------------
jfb wrote:
> Could you add a FIXME to implement `assignCalleeSavedSpillSlots` and `getCalleeSavedSpillSlots`? It looks like it's something we'll eventually want.
It's not clear yet whether we'll use the code that uses these. We can add them later if we need them.

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:37
@@ +36,3 @@
+
+  bool ForCodeSize;
+
----------------
jfb wrote:
> `OptimizeForSize` instead?
ForCodeSize is the name used in a few other places in LLVM. It's not just OptimizeForSize but also MinSize.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrAtomics.td:40
@@ +39,3 @@
+
+// Load-exclusives.
+
----------------
jfb wrote:
> Redundant.
Fixed.

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegisterInfo.h:27
@@ +26,3 @@
+class WebAssemblyRegisterInfo final {
+private:
+  const Triple &TT;
----------------
jfb wrote:
> No need for `private:` here.
Fixed.

================
Comment at: lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:48
@@ +47,3 @@
+                               ? "e-p:64:64-i64:64-v128:8:128-n32:64-S128"
+                               : "e-p:32:32-i64:64-v128:8:128-n32:64-S128",
+                        TT, CPU, FS, Options, RM, CM, OL),
----------------
jfb wrote:
> There doesn't seem to be a default mangling for datalayout, ELF seems like what we want: `m:e`.
> These are the defaults, but would be nice to specify: `f32:32-f64:64` ?
The default is MM_None, which is what I think we really want for now, unless we decide for sure we're using ELF.

I like leaving f32/f64 as defaults because it keeps the string shorter, and it makes it easier to see what the more interesting parts are.

http://reviews.llvm.org/D10569

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/

More information about the llvm-commits mailing list