[PATCH] D67384: [Attributor] Deduce memory behavior

[Johannes Doerfert via Phabricator via llvm-commits]

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

Any other objects or requests?



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:420
         return true;
+    if (IgnoreSubsumingPositions)
+      break;
----------------
uenoku wrote:
> 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.
I will make the behavior of the flag in the header clear and add a comment here.


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


================
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;
----------------
uenoku wrote:
> Maybe it is necessary to look at other atomic memory instructions also.
I will add a TODO. We should eventually have all instructions explicit here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67384