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

[Andy Wingo via Phabricator via llvm-commits]

wingo marked 6 inline comments as done.
wingo added inline comments.


================
Comment at: llvm/include/llvm/MC/MCContext.h:458
+    MCSymbol *getOrCreateWasmDefaultFunctionTableSymbol();
+
     /// @}
----------------
sbc100 wrote:
> 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?
I had the same thought and wanted to ask you about this.  FWIW I had these functions elsewhere until I settled on using the hook in `MC/WasmObjectWriter::recordRelocation`, at which point they need to be somewhere in MC/, right?

Backing up a bit -- all I really need is a hook called by WasmObjectWriter when it residualizes a relocation.  Perhaps there is a way to use MCAsmBackend to do this; I will take a look.


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


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:221
   unsigned NumGlobalImports = 0;
-  // NumTableImports is initialized to 1 to account for the hardcoded import of
-  // __indirect_function_table
----------------
sbc100 wrote:
> Move this comment down to reset?
No need, I think; there's no more hard-coded import of __indirect_function_table.  If it's imported, it will be present in the symtab.


================
Comment at: llvm/lib/Object/WasmObjectFile.cpp:565
         GlobalType = &Import.Global;
-        Info.ImportName = Import.Field;
         if (!Import.Module.empty()) {
----------------
sbc100 wrote:
> This seems unrelated?  What is going on here?
It is an unrelated cleanup to make this code the same as the other cases.  Beyond that, either this line or line 560 should be removed.


================
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"
----------------
sbc100 wrote:
> Why add these new headers to this file?
Sorry, was detritus from an earlier version of this change.  Removed.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:480
+    auto &Context = MF.getContext();
+    MIB.addSym(Context.getOrCreateWasmDefaultFunctionTableSymbol());
+    // Placeholder for immediate flags.
----------------
sbc100 wrote:
> 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).
Because I needed to mention __indirect_function_table from a number of places, I thought that perhaps I should avoid the possibility for typos.  We could put the string in another header or something though.


================
Comment at: llvm/test/CodeGen/WebAssembly/function-pointer64.ll:37
 ; CHECK-NEXT: i32.wrap_i64
-; CHECK-NEXT: call_indirect (i32) -> ()
+; CHECK-NEXT: call_indirect (i32) -> (), __indirect_function_table
 
----------------
aardappel wrote:
> Should this maybe be an implied default in both parsing and output?
I have now made it implied in parsing.  For output I don't know... my instinct would be that explicit is better in this case, but happy to change it if you think implicit is better.


================
Comment at: llvm/test/MC/WebAssembly/debug-info.ll:14
 ; CHECK-NEXT:    Type: IMPORT (0x2)
-; CHECK-NEXT:    Size: 81
 ; CHECK-NEXT:    Offset: 18
----------------
sbc100 wrote:
> 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?
Yeah I think that's the reason, yes.

Regarding patch splitting -- The issue is that this patch does two things:
 - makes call_indirect have a table operand, thus causing table relocs.  Also makes call_indirect take up more space because of sleb128 relocs.
 - emits the indirect function table import if and only if there are relocs against it (either implicitly via `TABLE_INDEX` or explicity via `TABLE_NUMBER`)

If you just do the first part, you have to then hack around to avoid a double-import of the indirect function table (the one that comes from the table operand, and the hard-coded import).  If you just do the second, you miss references from call_indirect.

I think this change may make sense as a unit, rather than split.  WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90948