[PATCH] D54647: [WebAssembly] Initial implementation of -fPIC codegeneration

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h:94
+  // similar to the GOT on native platforms.
+  MO_GOT = 1 << 3,
 
----------------
Is there a reason to use `1 << 3` instead of `0x8` to match the other enum values?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp:422
+
+    unsigned OpcConst = WebAssembly::CONST_I32;
+    unsigned ConstReg = createResultReg(&WebAssembly::I32RegClass);
----------------
Should this be dependent on `Subtarget->hasAddr64()` like `Opc` is?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp:428
+
+    unsigned OpcAdd = WebAssembly::ADD_I32;
+    BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(OpcAdd), Reg)
----------------
Same question for `OpcAdd`.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp:476
+  // For PIC code the global address in the stack already
+  if (GV && TLI.isPositionIndependent()) // && !TM.shouldAssumeDSOLocal(*GV->getParent(), GV))
+    IncludeGlobalAddr = false;
----------------
Should this commented code be deleted?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:765
+    //Opcode = Ins.empty() ? WebAssemblyISD::CALL_INDIRECT1 : WebAssemblyISD::CALL_INDIRECT1;
+  SDValue Res = DAG.getNode(Opcode, DL, InTyList, Ops);
   if (Ins.empty()) {
----------------
More commented code here.


================
Comment at: llvm/test/MC/WebAssembly/reloc-pic.s:1
+# RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=obj -o %t.o < %s
+# RUN: obj2yaml %t.o | FileCheck %s
----------------
If you want, you can avoid the temp file and output to stdout using `-o -`. I find that having the entire test on one line with no intermediate files makes the test easier to debug when something changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54647