[PATCH] D59625: [WebAssembly] Set "atomics" linkage depending on stripped ops or tls

[Thomas Lively via Phabricator via llvm-commits]

tlively marked an inline comment as done.
tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:260
+  auto &WasmTM = getWebAssemblyTargetMachine();
+  if (WasmTM.getUsedFeatures()[WebAssembly::FeatureAtomics]) {
     // Expand some atomic operations. WebAssemblyTargetLowering has hooks which
----------------
aheejin wrote:
> Does this work in case we don't specify `+matomics` in the command line but only some of functions contains `+matomics` in their function attributes? In that case the TM's `UsedFeatures` set will be updated as we go, but we query the info before we look into any functions here.
> 
> (I know it's preexisting; I think I didn't review the previous CL that added this part. And I may not have the full context of your recent target feature section related CLs, in which case this may be a dumb question)
You're right that this is kind of subtle and that all the used features are not known at this point, but I think that the code behaves reasonably as written. If `+atomics` is not provided to the TargetMachine then all atomics and tls will be stripped. If some function later on enables atomics, then atomics will be added to the WebAssemblyTargetMachine's `UsedFeatures`, but since all atomics will have already been stripped, the output will still not contain any atomics. Since atomics were stripped, the target feature section correctly gets `-atomics`, even though they were "used".

However, I think a better design would be to add an IR pass to precompute all of the features used in the module. This would allow me to remove the `mutable` qualifier from `UsedFeatures` and would make the `WebAssemblyTargetMachine` safe to use for multiple modules. It would also allow us to strip atomics and tls only if no function in the module enables atomics, which is more consistent with how we treat features in the target features section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59625