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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

More comments including various small style suggestions.

You also need to rewrite the commit message and the test case impact is missing.
For the commit message it is probably enough to drop the last part, thus:

> Introduce and deduce the "nosync" function attribute which indicates that a function does not synchronize with another thread in any way.

Remind me, is there a language ref patch for nosync somewhere? If not, we need to add a description in the LangRef.doc as well.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:337
 
 struct AANoSyncFunction : AbstractAttribute, BooleanState {
 
----------------
I forgot that before but I think it is better if we split it into a `struct AANoSync` in the header and a `struct AANoSyncFunction` in the cpp file. Some functions would go in the generic header struct, but not getState, getManifestPosition, updateImpl, and ID. This makes it easier to use the result in other attributes.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:352
   }
 
 
----------------
2 new lines.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:383
 
   /// Helper function used to get ordering of an atomic instruction.
   bool isNonRelaxedAtomic(Instruction *I) const;
----------------
Comment needs to be updated.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:384
   /// Helper function used to get ordering of an atomic instruction.
   bool isNonRelaxedAtomic(Instruction *I) const;
 
----------------
make it a static member function if possible. that way we can reuse it easier. Same for isVolatile.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:408
     break;
   case Instruction::AtomicCmpXchg:
     Success = cast<AtomicCmpXchgInst>(I)->getSuccessOrdering();
----------------
Given that Success and Failure are only needed in this case you can declare them here.
To do so you need to add brackets around the case:
```
case Instruction::AtomicCmpXchg: {
...
}
```


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:413
         Success == AtomicOrdering::Monotonic)
         return false;
     if (Failure == AtomicOrdering::Unordered ||
----------------
Why is it sufficient that one ordering is "weak enough"? Don't we have to test both? Either way, we need a comment to explain what is happening here.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:418
   default:
-    Ordering = AtomicOrdering::NotAtomic;
-
-    break;
-    // Relaxed.
-    if (Ordering == AtomicOrdering::Unordered ||
-        Ordering == AtomicOrdering::Monotonic)
-      return false;
     return true;
   }
----------------
No worries, all good. Please also add a comment to explain what this means and why we return true.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:420
   }
+    // Relaxed.
+  if (Ordering == AtomicOrdering::Unordered ||
----------------
Indention. And maybe add a few more words here ;)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:451
 
-    if ((!NoSyncAA || !NoSyncAA->isAssumedNoSync()) &&
+    if (!ICS && (!NoSyncAA || !NoSyncAA->isAssumedNoSync()) &&
         !ICS.hasFnAttr("nosync")) {
----------------
I think the`!` in front of ICS is a problem. Did you run this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766