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

[Andy Wingo via Phabricator via llvm-commits]

wingo added inline comments.


================
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:
> wingo wrote:
> > 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?
> I'm not clear why we can't to the second part first... why can't we emit the table if and only if there is a TABLE_INDEX relocation? 
> 
> Are you suggesting that there could be some call_indirect instruction generated by MC that doesn't have a TABLE_INDEX relocation attached to it?  
Ah I see what you mean, I was thinking that because we wouldn't have the table operand, we would miss out on the function table reference.  But TABLE_INDEX relocs implicitly causing a dep on the indirect function table would be sufficient, yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90948