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

Johannes Doerfert via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Jun 2 19:23:08 PDT 2019

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(); }

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?

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list