[PATCH] D153311: [Attributor] Unify AAMemoryLocation and AAMemoryBehavior

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:4539
+  unsigned NonFixMemoryEffects;
+  SmallVector<char> ArgMapping;
+  SmallVector<AA::MemoryEffects, 8> AssumedEffects, KnownEffects;
----------------
nikic wrote:
> Will overflow with many args?
Fair, I make it bigger.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:4539
+  unsigned NonFixMemoryEffects;
+  SmallVector<char> ArgMapping;
+  SmallVector<AA::MemoryEffects, 8> AssumedEffects, KnownEffects;
----------------
arsenm wrote:
> jdoerfert wrote:
> > nikic wrote:
> > > Will overflow with many args?
> > Fair, I make it bigger.
> If this is really a hard argument limit, it's way too small. llvm-reduce likes to move instructions to arguments, so I'm sure I'll run into something where this overflows in no time
I'll get rid of the indirection. AA::MemoryEffects is 32 bit, no real savings here even for "many" non-pointer args.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8332
+    case Instruction::Load:
+      return AA::MemoryEffects::readOnly();
+
----------------
nikic wrote:
> Does this handle volatile loads correctly? Those can also write (and access inaccessible memory).
This was already wrong in the old code, code catch. That said, volatile should IMHO not access inaccessible memory. That seems like a contradiction to me since it would be an accessible then. I can make it access (=read/write) everything, but I think we should revisit that.


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

https://reviews.llvm.org/D153311