[PATCH] D67384: [Attributor] Deduce memory behavior

[Hideto Ueno via Phabricator via llvm-commits]

uenoku added a comment.

Overall, I think the logic is sound.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:420
         return true;
+    if (IgnoreSubsumingPositions)
+      break;
----------------
It is based on the explicit rule that `SubsumingPositionIterator` must iterate the corresponding IRPosition first. I think you should mention this rule somewhere in  `SubsumingPositionIterator` implementation or header.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3228-3230
+  if ((S.getAssumed() & FnMemAA.getAssumed()) == S.getAssumed()) {
+    return ChangeStatus::UNCHANGED;
+  }
----------------
style: remove bracket


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3306-3312
+  case Instruction::Store:
+    // Stores cause the NO_WRITES property to disappear if the use is the
+    // pointer operand. Note that we do assume that capturing was taken care of
+    // somewhere else.
+    if (cast<StoreInst>(UserI)->getPointerOperand() == U->get())
+      removeAssumedBits(NO_WRITES);
+    return;
----------------
Maybe it is necessary to look at other atomic memory instructions also.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67384