[PATCH] D62766: [Attributor] Deduce "nosync" function attribute.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/docs/LangRef.rst:1479
+    (synchronize) with another thread causing that other thread to delete
+    the memory.
 ``nounwind``
----------------
The part after "causing" is too specific. We want nosync to be generic.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3
+// //
+// // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
----------------
Leftover comments?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:12
+// abstract interpretation style fixpoint iteration. See the Attributor.h
+// file comment and the class descriptions in that file for more information.
 //
----------------
(only a leftover of an old attributor patch version)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:170
+    StringRef StringAttr = Attr.getKindAsString();
+    if (StringAttr == Attr.getKindAsString())
+      NumFnNoSync++;
----------------
You have to check against "nosync".


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:378
+  /// Returns true of "nosync" is known.
+  bool isKnownNoFree() const { return getKnown(); }
+
----------------
Copy & paste


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:385
+/// helper functions.
+AtomicOrdering getOrdering(Instruction *I) {
+  switch (I->getOpcode()) {
----------------
Make it `static` or member function. Also, describe what it does in the comment.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:398
+
+bool isVolatile(Instruction *I) {
+  switch (I->getOpcode()) {
----------------
Description missing.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:418
+                          (unsigned)Instruction::Store,
+                          (unsigned)Instruction::AtomicRMW};
+
----------------
What about calls? Maybe you need to look at all side-effect instructions?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:442
+  }
+  indicateOptimisticFixpoint();
+  return ChangeStatus::UNCHANGED;
----------------
This is not a fixpoint. UpdateImpl is called multiple times (potentially). Remove the fixpoint call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766