[llvm-commits] [llvm] r55696 - in /llvm/trunk: lib/Analysis/IPA/GlobalsModRef.cpp test/Analysis/GlobalsModRef/2008-09-03-Mutual.ll test/Analysis/GlobalsModRef/2008-09-03-ReadGlobals.ll test/Analysis/GlobalsModRef/2008-09-03-ReadOnly.ll

[Chris Lattner]

On Sep 3, 2008, at 5:55 AM, Duncan Sands wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=55696&view=rev
> Log:
> Cleanup GlobalsModRef a bit.  When analysing the
> callgraph, when one member of a SCC calls another
> then the analysis would drop to mod-ref because
> there is (usually) no function info for the callee
> yet; fix this.  Teach the analysis about function
> attributes, in particular the readonly attribute
> (which requires being careful about globals).

Hey Duncan,

Very nice.  Do you have a specific interest/goal for GlobalsModRef, or  
is this just something you've notice that we do more poorly than we  
could?  Better alias analysis helps everything, so it's great to see  
progress in this area!

You've made a bunch of improvements to GlobalsModRef now and in the  
past, so I'm just going to go through and re-review it completely if  
that's ok with you.  Well I guess it doesn't matter if it's ok or not,  
because I just did it. :)  If you don't want to tackle something, let  
me know and I can add it to my queue.

   struct VISIBILITY_HIDDEN FunctionRecord {
     /// GlobalInfo - Maintain mod/ref info for all of the globals  
without
     /// addresses taken that are read or written (transitively) by this
     /// function.
     std::map<GlobalValue*, unsigned> GlobalInfo;

     /// MayReadAnyGlobal - May read global variables, but it is not  
known which.
     bool MayReadAnyGlobal;

...

     /// FunctionEffect - Capture whether or not this function reads  
or writes to
     /// ANY memory.  If not, we can do a lot of aggressive analysis  
on it.
     unsigned FunctionEffect;


Please keep the instance variables in the class together by moving  
getInfoForGlobal down, preferably after the ctor.

Also, can you shrink FunctionEffect to a 'char'?  That will reduce the  
size of FunctionRecord by a word, packing it next to the bool.


As far as efficiency goes, GlobalInfo is mysterious: it is really  
horrible as a std::map (one allocation per insertion) but making it a  
densemap wouldn't be any better (tons of fragmentation).  I think the  
best thing would be to eliminate it from FunctionRecord, and add a  
single unified DenseMap to the pass:

DenseMap<std::pair<Function*, GlobalVariable*>, unsigned>  
PerFunctionGlobalMRInfo;

This would work just fine for almost all uses and is the best of both  
worlds, the one problem would be the 'Incorporate callee's effects on  
globals into our info.' loop.  A solution to this would be to keep  
around the "per global" mod ref info for just the callees of the  
current SCC.  What do you think?



   class VISIBILITY_HIDDEN GlobalsModRef
       : public ModulePass, public AliasAnalysis {

Could this be a CallGraphSCCPass now?


NonAddressTakenGlobals/IndirectGlobals should really be SmallPtrSet's,  
and AllocsForIndirectGlobals should be a densemap.



    virtual ModRefBehavior getModRefBehavior(Function *F, CallSite CS,
                                           
std::vector<PointerAccessInfo> *Info) {
       if (FunctionRecord *FR = getFunctionInfo(F)) {
         if (FR->FunctionEffect == 0)
           return DoesNotAccessMemory;
         else if ((FR->FunctionEffect & Mod) == 0)
           return OnlyReadsMemory;
       }
       return AliasAnalysis::getModRefBehavior(F, CS, Info);
     }

Just because we think the function is readonly doesn't mean that a  
smarter analysis couldn't find that it is readnone.  How about:

         else if ((FR->FunctionEffect & Mod) == 0)
           return AliasAnalysis::getModRefBehavior(F, CS, Info) &  
OnlyReadsMemory;

?


/// getUnderlyingObject - This traverses the use chain to figure out  
what object
/// the specified value points to.  If the value points to, or is  
derived from,
/// a global object, return it.
static Value *getUnderlyingObject(Value *V) {

I think this is an exact dupe of Value::stripPointerCasts now:  
getUnderlyingObject(V) == V->stripPointerCasts();

If it needs to stay for some reason, then this statement is not needed:

   // If we are at some type of object... return it.
   if (GlobalValue *GV = dyn_cast<GlobalValue>(V)) return GV;


/// AnalyzeGlobals - Scan through the users of all of the internal
/// GlobalValue's in the program.  If none of them have their "address  
taken"

"If none of them" -> "If any of them"



   for (Module::global_iterator I = M.global_begin(), E =  
M.global_end();
        I != E; ++I)
     if (I->hasInternalLinkage()) {

How about: if (!I->hasInternalLinkage()) continue;


bool GlobalsModRef::AnalyzeUsesOfPointer(Value *V,
..
     } else if (CallInst *CI = dyn_cast<CallInst>(*UI)) {
       // Make sure that this is just the function being called, not  
that it is
       // passing into the function.
       for (unsigned i = 1, e = CI->getNumOperands(); i != e; ++i)
         if (CI->getOperand(i) == V) return true;

it would be faster to check UI.getOperandNo() instead of looping,  
likewise for invoke.


       if (CE->getOpcode() == Instruction::GetElementPtr ||
           CE->getOpcode() == Instruction::BitCast) {

This code handles bitcast constant exprs but not bitcast instrs.  :)   
Please add logic for bitcast instrs.



        if (F->getName() != "calloc") return false;   // Not calloc.

This should use F->isName("calloc")


void GlobalsModRef::AnalyzeCallGraph(CallGraph &CG, Module &M) {
...

       if (F->isDeclaration()) {

This needs a comment above it, something like:  If there is a  
prototype in the SCC, then it is either an intrinsic, or an external  
function, which may do just about anything.  We need to handle this  
separately from scanning the body of the function.


         // Try to get mod/ref behaviour from function attributes.
         if (F->doesNotAccessMemory()) {

I don't think this should be checking function attributes for readonly/ 
readnone.  Instead, it should chain to another alias analysis pass  
(which would end up at basicaa).  basicaa would then be the one to  
check the attribute.



       for (CallGraphNode::iterator CI = SCC[i]->begin(), E = SCC[i]- 
 >end();
            CI != E && !KnowNothing; ++CI)

Needs a comment: loop over all of the call graph edges out of this SCC  
node: i.e. all the called functions.

         if (Function *Callee = CI->second->getFunction()) {

How about:

         Function *Callee = CI->second->getFunction();
         if (Callee == 0) {
           KnowNothing = true;
          break;
       }
      ...

           // Can't say anything about it.  However, if it is inside  
our SCC,
             // then nothing needs to be done.
             CallGraphNode *CalleeNode = CG[Callee];
             if (std::find(SCC.begin(), SCC.end(), CalleeNode) ==  
SCC.end())
               KnowNothing = true;

As before, please use SmallPtrSet instead of std::find.  This is  
quadratic on large SCCs.


     // Scan the function bodies for explicit loads or stores.
     for (unsigned i = 0, e = SCC.size(); i != e && FunctionEffect !=  
ModRef;++i)
       for (inst_iterator II = inst_begin(SCC[i]->getFunction()),
              E = inst_end(SCC[i]->getFunction());
            II != E && FunctionEffect != ModRef; ++II)

I think that this should Instruction::mayReadMemory and friends.  It  
isn't handling the vaarg instruction, for example.  Also, using these  
predicates would have handled the volatile case correctly automatically.


GlobalsModRef::getModRefInfo(CallSite CS, Value *P, unsigned Size) {
   unsigned Known = ModRef;

Shouldn't this take advantage of the FunctionEffect for the function  
as well, even if P isn't a pointer to a known global?

-Chris