[PATCH] D30369: Allow None as a MemoryLocation to getModRefInfo, use it to start cleaning up interfaces and uses

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/Analysis/MemoryLocation.h:72
   static MemoryLocation get(const Instruction *Inst) {
+    const Optional<MemoryLocation> &Loc = MemoryLocation::getOrNone(Inst);
+    assert(Loc.hasValue() && "unsupported memory instruction");
----------------
This is using reference lifetime extension probably unintentionally, may be better to write it more directly as:

  Optional<MemoryLocation> Loc = ...;


================
Comment at: include/llvm/Analysis/MemoryLocation.h:73-74
+    const Optional<MemoryLocation> &Loc = MemoryLocation::getOrNone(Inst);
+    assert(Loc.hasValue() && "unsupported memory instruction");
+    return Loc.getValue();
+  }
----------------
Optional already has the assert in it, so unless you particularly want that assert text/message, you could write this as:

  return *MemoryLocation.getOrNone(Inst);


================
Comment at: include/llvm/Analysis/MemoryLocation.h:77-86
     if (auto *I = dyn_cast<LoadInst>(Inst))
       return get(I);
     else if (auto *I = dyn_cast<StoreInst>(Inst))
       return get(I);
     else if (auto *I = dyn_cast<VAArgInst>(Inst))
       return get(I);
     else if (auto *I = dyn_cast<AtomicCmpXchgInst>(Inst))
----------------
Consider a switch over the inst kind enum to reduce the work repeatedly done by dyn_cast (at least it seems like LLVM code often uses a switch over enum rather than chained dyn_cast - I don't actually know if the overhead is real)


================
Comment at: include/llvm/Analysis/MemoryLocation.h:79-86
     else if (auto *I = dyn_cast<StoreInst>(Inst))
       return get(I);
     else if (auto *I = dyn_cast<VAArgInst>(Inst))
       return get(I);
     else if (auto *I = dyn_cast<AtomicCmpXchgInst>(Inst))
       return get(I);
     else if (auto *I = dyn_cast<AtomicRMWInst>(Inst))
----------------
omit 'else' after return ( http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return )


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:146
     assert(!IsCall);
-    return Loc;
+    return Loc.getValue();
   }
----------------
You can write this as "return *Loc;" if you like.


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:149
 
+  const Optional<MemoryLocation> &getLocOrNone() const { return Loc; }
+
----------------
Should this assert !IsCall?


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:162-163
   union {
     ImmutableCallSite CS;
-    MemoryLocation Loc;
+    Optional<MemoryLocation> Loc;
   };
----------------
Does !IsCall imply that Loc.hasValue() ? (or can the "None" state of Loc be used in some cases) Judging by "getLoc" I'm guessing it does imply that.

If it does imply that, then Optional's hasValue tracking is wasted space. Wonder if there's a nice abstraction that should exist for this case, but I'm not sure.

I ask partly because the use of Optional maybe makes that invariant non-obvious so a better matched abstraction may make the code easier to understand. 


https://reviews.llvm.org/D30369