This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Simplify initializeSymbols and merge it with ObjFile::parse. NFC.
ClosedPublic

Authored by ruiu on Feb 27 2018, 5:39 PM.

Details

Summary

This patch simplifies initializeSymbols. Since that function is called
at the tail context of ObjFile::parse, and the function is called only
once from that function, that's effectively just a continuation of
ObjFile::parse. So this patch merge it with ObjFile::parse.

Event Timeline

ruiu created this revision.Feb 27 2018, 5:39 PM

My comments only relate to the removal of the members and other refactoring. Maybe you want land this without those extra parts and make that a separate change?

lld/wasm/InputFiles.cpp
46

Call this NumFuncs is a little deceptive since its the number of imported functions, which is doesn't include the defined functions.

Perhaps just inline these calls below to avoid the variable?

168

This reads backwards to me.

I'd prefer it to read "Assert that the symbol's element index is less than the number of imported functions"... which is the same thing as saying "Assert that the symbol points to an imported function rather than a defined one".

Why not keep the more meaningful "NumFunctionImports" name? Or perhaps just call it twice since the first call is inside of an assert (so won't effect release performance)?

ruiu added inline comments.Feb 27 2018, 5:59 PM
lld/wasm/InputFiles.cpp
168

The reason why I removed these member variables is because they don't do anything expensive and actually as cheap as member variables. They are defined as shown below.

uint32_t getNumImportedGlobals() const { return NumImportedGlobals; }
uint32_t getNumImportedFunctions() const { return NumImportedFunctions; }

So, what "caching" results of these functions does is to make extra copies of these member variables in our instances, which doesn't make much sense to me. I'm not worried about the cost of having extra copies, but if we have two copies of the same data, that is error prone.

ruiu added inline comments.Feb 27 2018, 6:04 PM
lld/wasm/InputFiles.cpp
168

Oh sorry I misread your comment. To me

uint32_t N = WasmObj->getNumImportedFunctions();
assert(Sym.Info.ElementIndex >= N);
return Functions[Sym.Info.ElementIndex - N];

is easier to read than

assert(Sym.Info.ElementIndex >= WasmObj->getNumImportedFunctions());
return Functions[Sym.Info.ElementIndex - WasmObj->getNumImportedFunctions()];

because the latter is longer, and it isn't obvious to me that the two function calls are actually the same.

The reason why I wrote N <= something instead of something >= N is because I'm always thinking on the number line. N is left if it is smaller than the other.

But that's just my preference. If you feel strongly about it I can revert it.

ruiu added inline comments.Feb 27 2018, 6:13 PM
lld/wasm/InputFiles.cpp
168

On second thought, I think we don't really need this assert. If assertion is enabled, out of bound should be caught by the container, and no explicit bound checking isn't needed.

ruiu updated this revision to Diff 136211.Feb 27 2018, 6:18 PM
  • inline get{Global,Data,Function}Symbol functions
ruiu updated this revision to Diff 136212.Feb 27 2018, 6:21 PM
  • update dumpInfo
sbc100 accepted this revision.Feb 27 2018, 6:24 PM

You are right, I misread the comparison. LGTM.

This revision is now accepted and ready to land.Feb 27 2018, 6:24 PM
This revision was automatically updated to reflect the committed changes.