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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 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};
----------------
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..)


================
Comment at: lld/wasm/Driver.cpp:680
+            ? symtab->addOptionalDataSymbol("__table_base32")
+            : nullptr;
   }
----------------
How about:

```
if (config->is64.getValueOr(false))
  WasmSym::definedTableBase32 = symtab->addOptionalDataSymbol("__table_base32");
```


================
Comment at: lld/wasm/InputElement.h:55
+  return ie;
+}
+
----------------
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.




================
Comment at: lld/wasm/Symbols.h:566
   static DefinedData *definedTableBase;
+  // 32-bit copy in wasm64 to work around init expr limitations.
+  static UndefinedGlobal *tableBase32;
----------------
Can we mention that this is workaround for lack or  https://github.com/WebAssembly/extended-const and can potentially be removed if/wehn this lands?



================
Comment at: lld/wasm/SyntheticSections.cpp:343
       writeUleb128(os, WasmSym::tableBase->getGlobalIndex(), "__table_base");
-
       // Add the table index to __table_base
----------------
Maybe keep these spacer lines?  Then seem harmless at worst.


================
Comment at: llvm/test/MC/WebAssembly/reloc-pic64.s:4
+
+# Verify that @GOT relocation entryes result in R_WASM_GLOBAL_INDEX_LEB against
+# against the corrsponding function or data symbol and that the corresponding
----------------
"entries" .. which looks one of my spelling mistakes that you cargo culted :)


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

https://reviews.llvm.org/D101784