[PATCH] D101784: [WebAssembly] Fix PIC/GOT codegen for wasm64

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 4 inline comments as done.
aardappel added inline comments.


================
Comment at: lld/wasm/Driver.cpp:587
   llvm::wasm::WasmGlobal wasmGlobal;
-  if (config->is64.getValueOr(false)) {
-    wasmGlobal.Type = {WASM_TYPE_I64, isMutable};
-    wasmGlobal.InitExpr.Opcode = WASM_OPCODE_I64_CONST;
-    wasmGlobal.InitExpr.Value.Int64 = 0;
-  } else {
-    wasmGlobal.Type = {WASM_TYPE_I32, isMutable};
-    wasmGlobal.InitExpr.Opcode = WASM_OPCODE_I32_CONST;
-    wasmGlobal.InitExpr.Value.Int32 = 0;
-  }
+  bool is64 = config->is64.getValueOr(false);
+  wasmGlobal.Type = {uint8_t(is64 ? WASM_TYPE_I64 : WASM_TYPE_I32), isMutable};
----------------
sbc100 wrote:
> Since `config->is64.getValueOr(false)` appears so often in the code I wonder if we could make `config->is64` just work (less typing and easier to read.. less likely to cache in a local variable).   I know we had a good reason for making it an optional... but maybe we should re-visit?
> 
> (not blocking this change just mentioning it..)
We had a single boolean, until we decided we needed to know wether it was set or not, which is exactly in one place. We could also just add a second `is64set` boolean or whatever, that way the other 15 or so uses can be simplified. Up to you.


================
Comment at: lld/wasm/InputElement.h:55
+  return ie;
+}
+
----------------
sbc100 wrote:
> This seems a little out of place here... although I don't know if we have a better home for such utilities.   If it was just used in one source file I would say move it out of the header.
> 
> 
It's used in 3 source files.


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

https://reviews.llvm.org/D101784