[PATCH] D95425: Implementation of global.get/set for reftypes in LLVM IR

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp:132-134
+    // Setting Comdat ensure only one table is left after linking when multiple
+    // modules define the table.
+    Sym->setComdat(true);
----------------
tlively wrote:
> Is this necessary given that the symbol is set to be undefined? Perhaps it would be better to make it defined here and also set comdat so that the linker doesn't need to do anything special to make sure it exists in the final binary. (Or possibly I'm misunderstanding what these settings mean!) @sbc100 wdyt?
I think don't want this here.  Firstly I think this will mark the symbol as being a comdat group symbol .. which is not the same thing as being part of a comdat group ( which I think is that is indented here).    Secondly, I don't think it makes sense for an undefined symbol to be part of a comdat group.  I think what you want here, if the symbol was defined is `setWeak()`.    Given that is not defined I dont think you even want that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425