[PATCH] D81704: [WebAssembly] Adding 64-bit version of R_WASM_MEMORY_ADDR_* relocs

[Wouter van Oortmerssen via Phabricator via llvm-commits]

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


================
Comment at: lld/wasm/Driver.cpp:502
   wasmGlobal.Type = {WASM_TYPE_I32, isMutable};
-  wasmGlobal.InitExpr.Value.Int32 = value;
+  wasmGlobal.InitExpr.Value.Int64 = value;
   wasmGlobal.InitExpr.Opcode = WASM_OPCODE_I32_CONST;
----------------
sbc100 wrote:
> This seems odd.
> 
> I wonder if this function should at least take an `in32_t`?
As I mentioned below, I have tried instead your approach of explicitly checking the opcode. I hope I have caught them all. Please review :)


================
Comment at: lld/wasm/InputChunks.cpp:89
         rel.Type != R_WASM_GLOBAL_INDEX_I32) {
-      uint32_t expectedValue = file->calcExpectedValue(rel);
+      auto expectedValue = file->calcExpectedValue(rel);
       if (expectedValue != existingValue)
----------------
sbc100 wrote:
> I don't think I'm ready for these kind of autos just yet.  I'd rather see what type it is.
The reason I replaced these with auto is we had a bunch of spots where we had unnecessary truncation because of a `uint32_t var = exp that results in a 64-bit value`. Using auto avoids these problems, now, and with future changes.


================
Comment at: lld/wasm/InputFiles.cpp:141
+  case R_WASM_MEMORY_ADDR_LEB64:
+>>>>>>> [WebAssembly] Adding 64-bit version of R_WASM_MEMORY_ADDR_* relocs
   case R_WASM_MEMORY_ADDR_SLEB:
----------------
sbc100 wrote:
> oops?
how did I miss that..


================
Comment at: llvm/include/llvm/BinaryFormat/Wasm.h:79
-    int32_t Int32;
     int64_t Int64;
     uint32_t Float32;
----------------
sbc100 wrote:
> How about calling this something else?  Maybe ConstVal?  Or just Int?  
> 
> The fact that you have left all the other ones makes me less attracted to this part of the change.    What would it looks like to instead leave this member of the union and instead check all accesses to ensure they match the Opcode?
Ok, as you wish. This added a bunch of new if-thens to the code, please have a look if they are correct. Also see the TODO(wvo).


================
Comment at: llvm/lib/Object/RelocationResolver.cpp:498
   case wasm::R_WASM_MEMORY_ADDR_I32:
+  case wasm::R_WASM_MEMORY_ADDR_I64:
   case wasm::R_WASM_TYPE_INDEX_LEB:
----------------
sbc100 wrote:
> But we don't support 64 bit relocs o wasm32 and vice versa, right?  I mean I don't think we should.
ok, I'll make 2 of these functions instead.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:854
       }
+      if (getSTI().getTargetTriple().getArch() == Triple::ArchType::wasm64) {
+        // Upgrade 32-bit loads/stores to 64-bit. These mostly differ by having
----------------
sbc100 wrote:
> Does the triple have something like is64() that might make this shorter?  Or isWasm64?  
Using `isArch64Bit` now.


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

https://reviews.llvm.org/D81704