[llvm-commits] [llvm] r56341 - in /llvm/trunk: include/llvm/ include/llvm/Transforms/ lib/Transforms/IPO/ test/Analysis/GlobalsModRef/ test/Transforms/AddReadAttrs/

[Duncan Sands]

Hi Chris,

> This pass can only do something if the SCC is not already readnone.   
> Does it make sense to do an initial check to see if the function is  
> already readnone?  Since it doesn't have to be a perfect check, how  
> about something like:
> 
> if (SCC.size() == 1 && SCC[0]->getFunction() &&
>      SCC[0]->getFunction()->doesNotAccessMemory())
>    return false;  // already readnone
> 
> Alternatively, maybe this should be handled by the 'already perfect'  
> case you already have.

I don't think this makes much sense, since in this case you just
whiz through without doing much.  All that happens is:

  A call to getAnalysis.
  Creation of a SmallPtrSet and placing one element in it.
  Getting the function from the CallGraphNode.
  A call to doesNotAccessMemory.
  Another getting of the function from CallGraphNode.
  Another call to doesNotAccessMemory.

Some of these are wrapped in loops of length 1, adds some
overhead.  I don't know how much a call to getAnalysis costs,
but the other costs should be tiny.

>      // Definitions with weak linkage may be overridden at linktime with
>      // something that writes memory, so treat them like declarations.
>      if (F->isDeclaration() || F->hasWeakLinkage()) {
> 
> This should also handle linkonce linkage.

It does now: it checks F->mayBeOverridden.

>        // Ignore calls to functions in the same SCC.
>        if (CS.getInstruction() &&
>            std::find(SCC.begin(), SCC.end(),  
> CG[CS.getCalledFunction()]) !=
>            SCC.end())
>          continue;
> 
> This will be really slow for large SCCs.  I think that some apps (like  
> 176.gcc) have scc's with hundreds of nodes in them.  Instead of using  
> std::find, please dump the contents of the SCC into a SmallPtrSet at  
> the start of the function, and query that instead.

Done.

>        if (II->mayWriteToMemory())
>          return false;
> 
> Please add a comment: "If this may write to memory, then we can't set  
> either readnone or readonly, just give up." or something like that.

Done.

Thanks for the review!

Ciao,

Duncan.