[PATCH] This patch introduces MemorySSA, a virtual SSA form for memory.Details on what it looks like are in MemorySSA.h

hfinkel at anl.gov hfinkel at anl.gov
Fri Feb 27 13:18:36 PST 2015


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:86
@@ +85,3 @@
+// This is mostly ripped from MemoryDependenceAnalysis, updated to
+// handle call instructions properly
+
----------------
What exactly does "updated to handle call instructions properly" mean? Also, this looks like it should be a member function of AA. Can you please add it there (then we can update MDA to use it too?).

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:100
@@ +99,3 @@
+  else if (auto *I = dyn_cast<CallInst>(Inst))
+    return AliasAnalysis::Location(I);
+  else if (auto *I = dyn_cast<InvokeInst>(Inst))
----------------
Part of my confusion is this: What does the call's return value have to do with the memory location(s) accessed by the call?

We do have these in AA:

  static Location getLocationForSource(const MemTransferInst *MTI);
  static Location getLocationForDest(const MemIntrinsic *MI);

and I imagine those would be good to use here.

But for generic calls, we don't have any facility to get a location, or list of locations, touched by the call. We can do call-site vs. call-site queries, and call-site vs. Location queries, however.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:136
@@ +135,3 @@
+  // 1. One argument dominates the other, and the other's version is a
+  // "false" one.
+  // 2. All of the arguments are false version numbers that eventually
----------------
Can you please define here what a "false" version is? (non-aliasing?)

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:241
@@ +240,3 @@
+          break;
+      } else if (AA->alias(getLocationForAA(AA, DefMemoryInst), Loc) !=
+                 AliasAnalysis::NoAlias)
----------------
I'm likely missing something here, but why don't you just call AA->getModRefInfo for all types of instructions? The overload for Instruction* handles all of the various types:

  /// getModRefInfo - Return information about whether or not an instruction may
  /// read or write the specified memory location.  An instruction
  /// that doesn't read or write memory may be trivially LICM'd for example.
  ModRefResult getModRefInfo(const Instruction *I,
                             const Location &Loc) {
    switch (I->getOpcode()) {
    case Instruction::VAArg:  return getModRefInfo((const VAArgInst*)I, Loc);
    case Instruction::Load:   return getModRefInfo((const LoadInst*)I,  Loc);
    case Instruction::Store:  return getModRefInfo((const StoreInst*)I, Loc);
    case Instruction::Fence:  return getModRefInfo((const FenceInst*)I, Loc);
    case Instruction::AtomicCmpXchg:
      return getModRefInfo((const AtomicCmpXchgInst*)I, Loc);
    case Instruction::AtomicRMW:
      return getModRefInfo((const AtomicRMWInst*)I, Loc);
    case Instruction::Call:   return getModRefInfo((const CallInst*)I,  Loc);
    case Instruction::Invoke: return getModRefInfo((const InvokeInst*)I,Loc);
    default:                  return NoModRef;
    }
  }


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:269
@@ +268,3 @@
+    ++NumClobberCacheHits;
+    DEBUG(dbgs() << "Cached Memory SSA version for " << (uintptr_t)I << " is ");
+    DEBUG(dbgs() << (uintptr_t)CCV->second);
----------------
Why are you printing the instruction addresses here? Would it be more useful to print *I?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:357
@@ +356,3 @@
+  unsigned ID = 0;
+  for (auto I = F.begin(), E = F.end(); I != E; ++I)
+    BBNumbers[I] = ID++;
----------------
Range-based for?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:411
@@ +410,3 @@
+
+      for (auto SI = succ_begin(BB), SE = succ_end(BB); SI != SE; ++SI) {
+        DomTreeNode *SuccNode = DT->getNode(*SI);
----------------
Range-based for using succ_range?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:435
@@ +434,3 @@
+
+      for (auto CI = Node->begin(), CE = Node->end(); CI != CE; ++CI) {
+        if (!Visited.count(*CI))
----------------
Range-based for here?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:490
@@ +489,3 @@
+  if (Accesses)
+    for (auto LI = Accesses->begin(), LE = Accesses->end(); LI != LE; ++LI) {
+      if (isa<MemoryPhi>(*LI))
----------------
Range-based for?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:539
@@ +538,3 @@
+    unsigned ChildLevel = DomLevels[Node] + 1;
+    for (auto CI = Node->begin(), CE = Node->end(); CI != CE; ++CI) {
+      DomLevels[*CI] = ChildLevel;
----------------
Range-based for?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:555
@@ +554,3 @@
+    return;
+
+  for (auto AI = Accesses->begin(), AE = Accesses->end(); AI != AE;) {
----------------
Maybe add here?

  assert(!DT->isReachableFromEntry(BB) &&
              "Reachable block found while handling unreachable blocks");

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:613
@@ +612,3 @@
+        MemoryAccessAllocator.Allocate<MemoryAccess *>(UseList->size());
+    for (auto UI = UseList->begin(), UE = UseList->end(); UI != UE; ++UI)
+      User->addUse(*UI);
----------------
Range-based for?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:635
@@ +634,3 @@
+
+  for (auto FI = F.begin(), FE = F.end(); FI != FE; ++FI) {
+    std::list<MemoryAccess *> *Accesses = nullptr;
----------------
Range-based for?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:637
@@ +636,3 @@
+    std::list<MemoryAccess *> *Accesses = nullptr;
+    for (auto BI = FI->begin(), BE = FI->end(); BI != BE; ++BI) {
+      bool use = false;
----------------
Range-based for?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:651
@@ +650,3 @@
+          Loc.Ptr = BI;
+        AliasAnalysis::ModRefResult ModRef = AA->getModRefInfo(BI, Loc);
+        if (ModRef & AliasAnalysis::Mod)
----------------
You're testing against an empty location? I don't think this is what you want, how about using this:

  /// getModRefBehavior - Return the behavior when calling the given call site.
  virtual ModRefBehavior getModRefBehavior(ImmutableCallSite CS);


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:687
@@ +686,3 @@
+  // We create an access to represent "live on entry", for things like
+  // arguments or users of globals. We do not actually insert it in to
+  // the IR.
----------------
in to -> into

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:688
@@ +687,3 @@
+  // arguments or users of globals. We do not actually insert it in to
+  // the IR.
+  BasicBlock &StartingPoint = F.getEntryBlock();
----------------
But you don't really insert anything into the IR proper. I'd rephrase this.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:717
@@ +716,3 @@
+    for (auto FI = F.begin(), FE = F.end(); FI != FE; ++FI) {
+      if (!Visited.count(FI)) {
+        markUnreachableAsLiveOnEntry(PerBlockAccesses, FI, Uses);
----------------
No need for { } here.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:729
@@ +728,3 @@
+  // Delete our access lists
+  for (auto DI = PerBlockAccesses.begin(), DE = PerBlockAccesses.end();
+       DI != DE; ++DI) {
----------------
Range-based for?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:737
@@ +736,3 @@
+  std::vector<std::list<MemoryAccess *> *> UseListsToDelete;
+  for (auto DI = Uses.begin(), DE = Uses.end(); DI != DE; ++DI) {
+    UseListsToDelete.push_back(DI->second);
----------------
Range-based for? And you don't need the { } here.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:770
@@ +769,3 @@
+void MemorySSA::verifyDefUses(Function &F) {
+  for (auto FI = F.begin(), FE = F.end(); FI != FE; ++FI) {
+    // Phi nodes are attached to basic blocks
----------------
Range-based for?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:780
@@ +779,3 @@
+    }
+    for (auto BI = FI->begin(), BE = FI->end(); BI != BE; ++BI) {
+      MA = getMemoryAccess(BI);
----------------
Range-based for?

http://reviews.llvm.org/D7864

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list