[PATCH] D59625: [WebAssembly] Merge used feature sets, update atomics linkage policy

[Thomas Lively via Phabricator via llvm-commits]

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:
> tlively wrote:
> > aheejin wrote:
> > > tlively wrote:
> > > > 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.
> > > I agree that would be probably a better and safer design. By the way is there any case we use an instance of `WebAssemblyTargetMachine` for multiple modules?
> > @dschuff just helped me investigate this, and it turns out `WebAssemblyTargetMachine` can be used to compile multiple modules. This means that we have to store the relevant information somewhere else. I believe attaching the target features as metadata to the Module itself is the best way to go. The Module is already supplied in the WebAssemblyAsmPrinter and obviously the Module is not reused between compilations. This will actually be a big code simplification!
> Just curious. In which case `WebAssemblyTargetMachine` is used to compile multiple modules? 
In llc there is an option to compile a module twice to make sure the results are the same. This option makes a clone of the input module and uses the same PassRunner (and therefore the same TargetMachine) to compile both of them. In that case this isn't actually a problem because the modules are the same, but it shows that some other tool (maybe a compile server) would be allowed to reuse the same TargetMachine for multiple modules.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:265
+        // Otherwise features are marked Used or not mentioned
+        M.addModuleFlag(Module::ModFlagBehavior::Error,
+                        (StringRef("wasm-feature-") + KV.Key).str(),
----------------
aheejin wrote:
> Why is this an error? If module A uses a feature and B does not use it, doesn't the merged module use it? So shouldn't this be `Module::ModFlagBehavior::Override` too?
The Error behavior only kicks in if two modules with different values for the same flag are merged. If a module with some flag is merged with another module without that flag, then there is no error. Since I do not emit metadata for unused features, this should not be a problem. Thinking about this more, I think the Override behavior for disallowed atomics should actually be Error to avoid a situation where a merged module uses atomics but also has atomics marked disallowed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59625