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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Some more comments but it looks much better already. The test changes are missing though.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:377
+
+  /// Returns true of "nosync" is known.
+  bool isKnownNoSync() const { return getKnown(); }
----------------
"if"


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:388
+
+/// Helper function used to get ordering of an atomic instruction.
+AtomicOrdering AANoSyncFunction::getOrdering(Instruction *I) const {
----------------
Put the documentation on the declaration.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:406
+      return Failure;
+    return Success;
+  default:
----------------
Can you describe the logic here?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:435
+    ImmutableCallSite ICS(I);
+    auto *NoSyncAA = A.getAAFor<AANoSyncFunction>(*this, *I);
+
----------------
Only do the stuff below if `I` is actually a call, so if `ICS` is not `null`. If you run this, it should crash on you right now because you access ICS unconditionally.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:438
+    if ((!NoSyncAA || !NoSyncAA->isAssumedNoSync()) &&
+        !ICS.hasFnAttr("nosync") && !ICS.hasFnAttr("readnone")) {
+      indicatePessimisticFixpoint();
----------------
I think `getReadOrWriteInstsForFunction` will never pick up a `readnone` call as it will neither read nor write memory.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:450
+         Ordering == AtomicOrdering::Monotonic) &&
+        !isVolatile(I))
+      continue;
----------------
Can we have a single call to `isVolatile`, maybe always call that one and `getOrdering` and decide on the result what to do. That would mean move `I->isAtomic()` into `getOrdering()` and ensure we catch all opcodes in the switch (so the default prints an error)..


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:600
+    case Instruction::AtomicRMW:
+    case Instruction::Fence:
+      IsInterestingOpcode = true;
----------------
sstefan1 wrote:
> Just to make sure, when using `InformationCache::getReadOrWriteInstsForFunction` I don't need this, right?
Correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766