[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