[PATCH] D91803: [lld] Use -1 as tombstone value for discarded code ranges

[Eric Leese via Phabricator via llvm-commits]

Eric added inline comments.


================
Comment at: lld/test/wasm/debug-removed-fn.ll:7
 ; CHECK: 0x0000000000000005
-; CHECK: 0x0000000000000000
+; CHECK: 0x00000000ffffffff
 
----------------
dblaikie wrote:
> This looks suspcious - is something in the implementation (or the dumper) truncating the 64 bit -1 to a 32 bit -1? Or is this a bug in the dumper that it's printing out a 32bit value as a 64 bit value?
Looks like this file is coming from a R_WASM_FUNCTION_OFFSET_I32 type relocation so this is being written as 32 bits. I assume the dumper casts to 64 bits whether it read a 32 or 64 bit address. 


================
Comment at: lld/wasm/InputChunks.h:230
   const WasmSection §ion;
+  const uint64_t tombstone_value;
 };
----------------
sbc100 wrote:
> MaskRay wrote:
> > sbc100 wrote:
> > > MaskRay wrote:
> > > > sbc100 wrote:
> > > > > We use the lowerCamel style so I think this should be tombstoneValue?
> > > > Is the space consumption a concern? 
> > > I not been to concerted about adding data O(input chunk).   Honestly, we have not done much in the way optimizing wasm-ld for speed or memory consumption.   Rui often guided me to keep the symbol size down and to avoid per-symbol hash lookup.. but per-input-chunk data I've not been too concerned about.
> > > 
> > > I guess one alternative would be to store a tombstone-type enum with three possible values?   
> > The sizes of SymbolUnion and InputSections are important for the ELF's port.
> > 
> > As an example, the basic block sections feature cost us more than 1% peak memory usage. D91018 claimed back 0.6%
> Point taken.   And since wasm mandates `-ffunction-sections` and `-fdata-sections` is the default we are even more effected than ELF by default I guess.
> 
> I'm embarrassed to say I have done zero benchmarking of wasm-ld thus far.
Note that this is not in InputChunk, it is in InputSection, so this is once per custom wasm section, of which there might be what, a dozen? I could get rid of it but then we would need to do 1-3 string comparisons against the section name for every relocation in that section, which I was concerned could be costly, though I also have done no wasm-ld benchmarking.

I actually considered putting it in InputChunk so as to avoid lots of virtual function calls. If I did that this would cost us a megabyte for especially large applications.


================
Comment at: lld/wasm/InputFiles.cpp:196
+    if ((isa<FunctionSymbol>(sym) || isa<DataSymbol>(sym)) && !sym->isLive()) {
+      return tombstone ? tombstone : reloc.Addend;
+    }
----------------
dblaikie wrote:
> sbc100 wrote:
> > So setting the `tombstone` to zero means we should include the `Addend` ?
> Yeah, I guess that's beyond my debug info purview - but I'd suggest checking what bfd ld does here & maybe going with that, rather than what gold does/lld used to do. Which is probably absolute 0 rather than addend. For broader compatibility (& probably make this code a bit less subtle - avoiding the tombstone of zero not really being zero)
Yes, this was based on MaskRay@'s comment on my initial commit. If there is not agreement on this perhaps bring discussion back to the email thread?


================
Comment at: lld/wasm/InputFiles.h:106
   uint32_t calcNewIndex(const WasmRelocation &reloc) const;
-  uint64_t calcNewValue(const WasmRelocation &reloc) const;
+  uint64_t calcNewValue(const WasmRelocation &reloc, uint64_t tombstone = 0) const;
   uint64_t calcNewAddend(const WasmRelocation &reloc) const;
----------------
sbc100 wrote:
> It seems like all of the callsites should probably include the tombstone no?
> 
> Perhaps remove the default and specify it at each call site?
This felt different to me because zero has a unique meaning, it is not used as a tombstone it means we don't do tombstoning. I don't think adding ", 0" to the other callsites would make the code any clearer but of course I can do that if there is a strong preference for it.


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

https://reviews.llvm.org/D91803