[PATCH] D82130: [WebAssembly] Adding 64-bit versions of __stack_pointer and other globals

[Sam Clegg via Phabricator via cfe-commits]

sbc100 accepted this revision.
sbc100 added a comment.
This revision is now accepted and ready to land.

Great!



================
Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:68
+  else
+    CmdArgs.push_back("-m wasm32");
+
----------------
All the other drivers seem to do `CmdArgs.push_back("-m");` .. CmdArgs.push_back("xxx");

I would expect these to be two distinct argument..  in fact I'm not sure how it works with once arg that contains a space like this.


================
Comment at: lld/wasm/Driver.cpp:385
+    StringRef s = arg->getValue();
+    if (s == "wasm32")
+      config->is64 = false;
----------------
aardappel wrote:
> dschuff wrote:
> > any particular reason this shouldn't use the more conventional `-m32`/`m64`?
> > edit: nevermind, this is lld not clang. I'll defer to Sam's opinion on lld flags.
> Yup, @sbc100 suggested this.
Yeah it looks like gnu ld and lld elf always accept `-m <arch>` from clang.


================
Comment at: lld/wasm/Driver.cpp:390
+    else
+      error("'" + s + "' not a valid target");
+  }
----------------
It looks like lld ELF inherits the GNU ld terminology and calls this argument "emulation".   

```
      -m emulation
           Emulate the emulation linker.  You can list the available emulations with the
           --verbose or -V options.

           If the -m option is not used, the emulation is taken from the "LDEMULATION"
           environment variable, if that is defined.

           Otherwise, the default emulation depends upon how the linker was configured.
````

Pretty odd choice of name I think and we probably are ok no to copy that here.

But how about I think I prefer the term "architecture" to target.  Seems a little more specific.  

How about: `error("invalid target architecture: " + s);`


================
Comment at: lld/wasm/InputChunks.cpp:338
 
+  auto WASM_OPCODE_PTR_CONST =
+      config->is64 ? WASM_OPCODE_I64_CONST : WASM_OPCODE_I32_CONST;
----------------
Just use normal variable names here?  `opcode_const` and `opcode_add`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82130/new/

https://reviews.llvm.org/D82130