[llvm-commits] [llvm] r37958 - in /llvm/trunk: include/llvm/Analysis/MemoryDependenceAnalysis.h lib/Analysis/MemoryDependenceAnalysis.cpp

Chris Lattner clattner at apple.com
Sun Jul 8 18:56:39 PDT 2007


> A first stab at memory dependence analysis.  This is an interface  
> on top of
> alias analysis, adding caching and lazy computation of queries.   
> This will
> be used in planned improvements to memory access optimizations.

Woot!  Some thoughts:

> +#ifndef LLVM_ANALYSIS_MEMORY_DEPENDENCE_H
> +#define LLVM_ANALYSIS_MEMORY_DEPENDENCE_H
> +
> +#include "llvm/Pass.h"
> +#include "llvm/ADT/DenseMap.h"
> +#include "llvm/Support/Compiler.h"
> +#include <map>
> +
> +namespace llvm {
> +
> +class Function;
> +class FunctionPass;
> +class Instruction;
> +
> +class VISIBILITY_HIDDEN MemoryDependenceAnalysis : public  
> FunctionPass {

Please don't use VISIBILITY_HIDDEN for passes defined in headers.   
This should only be used for things in anonymous namespaces.  With  
this gone, you don't need compiler.h.

> +  private:
> +
> +    DenseMap<Instruction*, std::pair<Instruction*, bool> >  
> depGraphLocal;
> +    std::multimap<Instruction*, Instruction*> reverseDep;
> +
> +  public:
> +
> +    static Instruction* NonLocal;
> +    static Instruction* None;
> +
> +    static char ID; // Class identification, replacement for typeinfo
> +    MemoryDependenceAnalysis() : FunctionPass((intptr_t)&ID) {}
> +
> +    /// Pass Implementation stuff.  This doesn't do any analysis.
> +    ///
> +    bool runOnFunction(Function &) {
> +      depGraphLocal.clear();
> +      reverseDep.clear();
> +      return false;
> +    }

The map clearing should be done in releaseMemory.

> +    /// getAnalysisUsage - Does not modify anything.  It uses  
> Value Numbering
> +    /// and Alias Analysis.
> +    ///
> +    virtual void getAnalysisUsage(AnalysisUsage &AU) const;
> +
> +    /// getDependency - Return the instruction on which a memory  
> operation
> +    /// depends.
> +    Instruction* getDependency(Instruction* query, bool local =  
> true);

The comment should describe what "local" means.

> +/// getDependency - Return the instruction on which a memory  
> operation
> +/// depends.  NOTE: A return value of NULL indicates that no  
> dependency
> +/// was found in the parent block.
> +Instruction* MemoryDependenceAnalysis::getDependency(Instruction*  
> query,
> +                                                     bool local) {
> +  if (!local)
> +    assert(0 && "Non-local memory dependence is not yet supported.");
> +
> +  // Start looking for dependencies with the queried inst
> +  BasicBlock::iterator QI = query;
> +
> +  // Check for a cached result
> +  std::pair<Instruction*, bool> cachedResult = depGraphLocal[query];
> +  // If we have a _confirmed_ cached entry, return it
> +  if (cachedResult.second)
> +    return cachedResult.first;
> +  else if (cachedResult.first != NonLocal)
> +  // If we have an unconfirmed cached entry, we can start our  
> search from there
> +    QI = cachedResult.first;
> +
> +  AliasAnalysis& AA = getAnalysis<AliasAnalysis>();
> +
> +  BasicBlock::iterator blockBegin = query->getParent()->begin();

Please sink the definition of this variable down to right before the  
loop.

> +  // Get the pointer value for which dependence will be determined
> +  Value* dependee = 0;

You need to track the size of the pointer here.  The issue is that  
"free" frees the entire object regardless of the type provided.  For  
example, in this code, we want the free to depend on the store:
   int *P;
   store X into P[4]
   free P

To do this, the "free" case should set the size of the access to ~0U,  
which indicates "whole object" to the alias analysis stuff.

> +  if (StoreInst* S = dyn_cast<StoreInst>(QI))
> +    dependee = S->getPointerOperand();
> +  else if (LoadInst* L = dyn_cast<LoadInst>(QI))
> +    dependee = L->getPointerOperand();
> +  else if (FreeInst* F = dyn_cast<FreeInst>(QI))
> +    dependee = F->getPointerOperand();
> +  else if (isa<AllocationInst>(query)) {
> +    // Allocations don't depend on anything
> +    depGraphLocal.insert(std::make_pair(query, std::make_pair(None,
> +                                                               
> true)));
> +    reverseDep.insert(std::make_pair(None, query));
> +    return None;
> +  } else {
> +    // Non-memory operations depend on their immediate predecessor
> +    --QI;
> +    depGraphLocal.insert(std::make_pair(query, std::make_pair(QI,  
> true)));
> +    reverseDep.insert(std::make_pair(QI, query));
> +    return QI;

Non-memory instructions should depend on nothing, like allocation  
instructions.  Also, there is no need to insert trivial queries like  
these into the map.

Also, what about call/invoke instructions?

> +  }
> +
> +  // Start with the predecessor of the queried inst
> +  --QI;
> +
> +  TargetData& TD = getAnalysis<TargetData>();
> +
> +  while (QI != blockBegin) {

This loop isn't right, because you don't check the first instruction  
in the block.  I'd suggest something like this (remove the  
predecrement above also):

while (QI != blockbegin) {
   --QI;
   if (...) {
   } else if (...) {
   } else
     continue;

   check alias query here.
}


> +    // If this inst is a memory op, get the pointer it accessed
> +    Value* pointer = 0;

This also needs a size, for similar reasons above.
> +    if (StoreInst* S = dyn_cast<StoreInst>(QI))
> +      pointer = S->getPointerOperand();
> +    else if (LoadInst* L = dyn_cast<LoadInst>(QI))
> +      pointer = L->getPointerOperand();
> +    else if (isa<AllocationInst>(QI))
> +      pointer = QI;

Allocations with a constant array size can pass in the actual size of  
the allocation to AA, other allocations should pass in ~0U as the  
allocation size.

> +    else if (FreeInst* F = dyn_cast<FreeInst>(QI))
> +      pointer = F->getPointerOperand();
> +    else if (CallInst* C = dyn_cast<CallInst>(QI)) {
> +      // Call insts need special handling.  Check is they can  
> modify our pointer

Invoke is the same as call.  The CallSite class lets you handle both  
with one set of code.

> +      if (AA.getModRefInfo(C, dependee, TD.getTypeSize(dependee- 
> >getType())) !=
> +          AliasAnalysis::NoModRef) {
> +        depGraphLocal.insert(std::make_pair(query, std::make_pair 
> (C, true)));
> +        reverseDep.insert(std::make_pair(C, query));
> +        return C;
> +      } else {
> +        continue;
> +      }
> +    }
> +
> +    // If we found a pointer, check if it could be the same as our  
> pointer
> +    if (pointer) {
> +      AliasAnalysis::AliasResult R = AA.alias(
> +                                 pointer, TD.getTypeSize(pointer- 
> >getType()),
> +                                 dependee, TD.getTypeSize(dependee- 
> >getType()));

Another good thing about this change is that it eliminations the  
recomputation of dependee's size for each instruction :)

-Chris





More information about the llvm-commits mailing list