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

[Thomas Lively via Phabricator via llvm-commits]

tlively planned changes to this revision.
tlively marked 5 inline comments as done.
tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:237
+      assert(!UsedFeatures[WebAssembly::FeatureAtomics]);
+      Entry.Prefix = wasm::WASM_FEATURE_PREFIX_DISALLOWED;
+      EmittedFeatures.push_back(Entry);
----------------
aheejin wrote:
> Then when is the atomics feature 'required' (`WASM_FEATURE_PREFIX_REQUIRED`) now? Shouldn't it be required when it is in the used the and `WasmTM.getAtomicsStripped()` is false?
Currently WASM_FEATURE_PREFIX_REQUIRED is not used for anything. Originally we were using it as you described, but the problem is that if normal code with atomic ops has =atomics then it would not be linkable with thread-neutral libs like libpng. Using +atomics and -atomics instead of =atomics and -atomics is still safe with respect to stripped atomic ops, but also allows for thread-neutral objects.

We could entirely remove WASM_FEATURE_PREFIX_REQUIRED without breaking anything, but some people (Dan mostly) have expressed interest in a mode where all used features are strictly required. I'm not convinced that's useful, but I don't want to close the door on it either. Another potential use for WASM_FEATURE_PREFIX_REQUIRED would be enforcing ABI compatibility.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblySubtarget.h:84
   const Triple &getTargetTriple() const { return TargetTriple; }
+  bool enableAtomicExpand() const override;
   bool enableMachineScheduler() const override;
----------------
aheejin wrote:
> Where is this function used?
Called by AtomicExpand::runOnFunction in AtomicExpandPass.cpp to determine whether any work needs to be done for that function. This lets us unconditionally run the AtomicExpandPass when we are setting up IR passes and defer the decision about whether or not to the expand atomics to pass run time.


================
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:
> > > 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!


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:202
+      if (Features[KV.Value])
+        Ret += (StringRef("+") + KV.Key + ",").str();
+    }
----------------
aheejin wrote:
> Does this mean the features string always ends with a comma?
Yes. This doesn't appear to change its semantics and it makes the code slightly simpler.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:210
+    F.removeFnAttr("target-cpu");
+    F.addFnAttr("target-features", Feature);
+  }
----------------
aheejin wrote:
> - Do we need to do this to unify features between functions?
> - Why do we remove `target-cpu`?
Yes, this is the part where we unify all the features. We remove `target-cpu` because any features it has enabled will be represented in the new `target-features` string, so it is redundant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59625