[PATCH] D90948: [WebAssembly] [WIP] call_indirect issues table number relocs

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: llvm/include/llvm/MC/MCContext.h:458
+    MCSymbol *getOrCreateWasmDefaultFunctionTableSymbol();
+
     /// @}
----------------
Seems unfortunate to add this wasm-specific stuff to a generic header. 

I know this header already has some per-ojbect-format functions but they are generally limited to functions that exists for all formats  (e.g. `get<format>Section()`).

Is there somewhere target-specific we can put these?


================
Comment at: llvm/lib/MC/MCWasmStreamer.cpp:154
-  MCObjectStreamer::emitValueImpl(Value, Size, Loc);
-}
-
----------------
Looks like this is just a pointless override?   Can we land that separately (no need to wait for this change)?


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:1277
+        Import.Table.ElemType = uint8_t(ElemType);
+        // FIXME: ?
+        Import.Table.Limits = {wasm::WASM_LIMITS_FLAG_NONE, 0, 0};
----------------
I guess this comment could say say something like: "Extent table type to include limits?.    For not we don't specify a min or max which does not place any restrictions on the size of the imported table"  ?


================
Comment at: llvm/lib/Object/WasmObjectFile.cpp:565
         GlobalType = &Import.Global;
-        Info.ImportName = Import.Field;
         if (!Import.Module.empty()) {
----------------
This seems unrelated?  What is going on here?


================
Comment at: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp:23
 #include "llvm/MC/MCSymbolWasm.h"
+#include "llvm/MC/MCWasmStreamer.h"
 #include "llvm/Support/Casting.h"
----------------
Why add these new headers to this file?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:480
+    auto &Context = MF.getContext();
+    MIB.addSym(Context.getOrCreateWasmDefaultFunctionTableSymbol());
+    // Placeholder for immediate flags.
----------------
For other magic globals such as `__stack_pointer` we just hardcode the symbol name.   See `WebAssemblyFrameLowering::writeSPToGlobal`

I wonder if that is simpler than messing with the target independent MCContext.h?   (I'm not sure just wondering).


================
Comment at: llvm/test/MC/WebAssembly/debug-info.ll:14
 ; CHECK-NEXT:    Type: IMPORT (0x2)
-; CHECK-NEXT:    Size: 81
 ; CHECK-NEXT:    Offset: 18
----------------
Is this because we don't import that table anymore?

I wonder if this change, along with all the other test changes that are just about removing the table-by-default can land first.

i.e. can we make a change "don't emit table import in object files that don't have table relocation".    

Just in the interest of shrinking this PR?


================
Comment at: llvm/test/MC/WebAssembly/type-index.s:9
     .functype   test0 (i32) -> (i32)
-    call_indirect (f64) -> (f64)
+    call_indirect (f64) -> (f64), __indirect_function_table
     end_function
----------------
It seems reasonable to me that we should default to `__indirect_function_table`,   at least when parsing assembly.     i.e. this change should not be necessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90948