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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Mostly LGTM. Some nits and questions:



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:237
+      assert(!UsedFeatures[WebAssembly::FeatureAtomics]);
+      Entry.Prefix = wasm::WASM_FEATURE_PREFIX_DISALLOWED;
+      EmittedFeatures.push_back(Entry);
----------------
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?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblySubtarget.h:84
   const Triple &getTargetTriple() const { return TargetTriple; }
+  bool enableAtomicExpand() const override;
   bool enableMachineScheduler() const override;
----------------
Where is this function used?


================
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
----------------
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?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:202
+      if (Features[KV.Value])
+        Ret += (StringRef("+") + KV.Key + ",").str();
+    }
----------------
Does this mean the features string always ends with a comma?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:207
+
+  void replaceFeatures(Function &F, const std::string& Feature) {
+    F.removeFnAttr("target-features");
----------------
clang-format?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:210
+    F.removeFnAttr("target-cpu");
+    F.addFnAttr("target-features", Feature);
+  }
----------------
- Do we need to do this to unify features between functions?
- Why do we remove `target-cpu`?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.h:50
 
+  void setUsedFeatures(FeatureBitset Features) { UsedFeatures = Features; }
   FeatureBitset getUsedFeatures() const { return UsedFeatures; }
----------------
The argument here can be `const FeatureBitSet &`


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.h:51
+  void setUsedFeatures(FeatureBitset Features) { UsedFeatures = Features; }
   FeatureBitset getUsedFeatures() const { return UsedFeatures; }
+  void setAtomicsStripped() { AtomicsStripped = true; }
----------------
If this function is marked `const`, I guess it is supposed to return `const FeatureBitSet &`, considering now `UsedFeatures` is not a `mutable` member anymore?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.h:52
   FeatureBitset getUsedFeatures() const { return UsedFeatures; }
+  void setAtomicsStripped() { AtomicsStripped = true; }
+  bool getAtomicsStripped() const { return AtomicsStripped; }
----------------
We may not need that now, but probably this can be
```
void setAtomicsStripped(bool Value = true) { AtomicsStripped = V; }
```
to enable turning off the feature too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59625