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

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 marked 3 inline comments as done.
sstefan1 added a comment.

Tests almost done. I'll update in couple hours.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:406
+      return Failure;
+    return Success;
+  default:
----------------
jdoerfert wrote:
> Can you describe the logic here?
I had to return one. So if `Success` isn't intresting it returns `Failure` ordering. Ohterwise it doesn't matter since `Success` already syncs. I didn't give this much tought, if you have any suggestions, I'll apply them.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:438
+    if ((!NoSyncAA || !NoSyncAA->isAssumedNoSync()) &&
+        !ICS.hasFnAttr("nosync") && !ICS.hasFnAttr("readnone")) {
+      indicatePessimisticFixpoint();
----------------
jdoerfert wrote:
> I think `getReadOrWriteInstsForFunction` will never pick up a `readnone` call as it will neither read nor write memory.
So is it safe to remove it then, as it does not have side-effects?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:450
+         Ordering == AtomicOrdering::Monotonic) &&
+        !isVolatile(I))
+      continue;
----------------
jdoerfert wrote:
> 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)..
If I do it this way, I think it would be better to change `AtomicOrdering getOrdering()` to `bool isSyncOrdering()` or whatever is appropriate for the name. It can than return true if ordering is not Unordered or Monotonic. That way everything can be checked in one if. 

> and ensure we catch all opcodes in the switch (so the default prints an error)..
I only miss GetElementPtr and alloca which are not of great interest here, if I'm not wrong. But I can add them as well.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766