[llvm-commits] [poolalloc] r159572 - /poolalloc/trunk/lib/AssistDS/DSNodeEquivs.cpp

[John Criswell]

On 7/2/12 3:18 PM, Matthew Wala wrote:
> On Mon, Jul 2, 2012 at 2:59 PM, John Criswell <criswell at illinois.edu> wrote:
>> On 7/2/12 2:51 PM, Matthew Wala wrote:
>>> Author: wala1
>>> Date: Mon Jul  2 14:51:52 2012
>>> New Revision: 159572
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=159572&view=rev
>>> Log:
>>> Fix DSNodeEquivs to handle constant expressions; find some DSGraph an
>>> expression belongs to by looking through its uses.
>>>
>>> Modified:
>>>       poolalloc/trunk/lib/AssistDS/DSNodeEquivs.cpp
>>>
>>> Modified: poolalloc/trunk/lib/AssistDS/DSNodeEquivs.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/poolalloc/trunk/lib/AssistDS/DSNodeEquivs.cpp?rev=159572&r1=159571&r2=159572&view=diff
>>>
>>> ==============================================================================
>>> --- poolalloc/trunk/lib/AssistDS/DSNodeEquivs.cpp (original)
>>> +++ poolalloc/trunk/lib/AssistDS/DSNodeEquivs.cpp Mon Jul  2 14:51:52 2012
>>> @@ -17,6 +17,9 @@
>>>    #include "llvm/Constants.h"
>>>    #include "llvm/Module.h"
>>>    #include "llvm/Support/InstIterator.h"
>>> +#include "llvm/ADT/SmallSet.h"
>>> +
>>> +#include <deque>
>>>      namespace llvm {
>>>    @@ -233,6 +236,46 @@
>>>      } else if (isa<Argument>(V)) {
>>>        const Function *Parent = cast<Argument>(V)->getParent();
>>>        NHForV = &TDDS.getDSGraph(*Parent)->getNodeForValue(V);
>>> +  } else {
>>> +    //
>>> +    // Iterate over the users to attempt to discover a DSNode that the
>>> value
>>> +    // maps to.
>>> +    //
>>> +    std::deque<const User *> WL;
>>> +    SmallSet<const User *, 8> Visited;
>>> +
>>> +    WL.insert(WL.end(), V->use_begin(), V->use_end());
>>> +    do {
>>> +      const User *TheUser = WL.front();
>>> +      WL.pop_front();
>>> +
>>> +      if (Visited.count(TheUser))
>>> +        continue;
>>> +      else
>>> +        Visited.insert(TheUser);
>>
>> You should check and see if the insert method can tell you whether the
>> insert succeeded because the element was not in the set or whether the
>> insert failed because the element was already in the set.  This feature
>> allows you to do a single operation on the set and should speed up the
>> analysis.
>>
>>> +
>>> +      //
>>> +      // If the use is a global variable or instruction, then the value
>>> should
>>> +      // be in the corresponding DSGraph.
>>> +      //
>>> +      // TODO: Is it possible for a value to belong to some DSGraph and
>>> never
>>> +      // be relied upon by a GlobalValue or Instruction?
>>> +      //
>>> +      if (isa<Instruction>(TheUser)) {
>>
>> Why not use dyn_cast<Instruction> here?  That would avoid the
>> cast<Instruction> in the line below.
>>
>>> +        const Function *Parent =
>>> +          cast<Instruction>(TheUser)->getParent()->getParent();
>>> +        NHForV = &TDDS.getDSGraph(*Parent)->getNodeForValue(V);
>>> +        break;
>>> +      } else if (isa<GlobalValue>(TheUser)) {
>>> +        NHForV = &TDDS.getGlobalsGraph()->getNodeForValue(V);
>>> +        break;
>>> +      } else {
>>> +        //
>>> +        // If this use is of some other nature, look at the users of this
>>> use.
>>> +        //
>>> +        WL.insert(WL.end(), TheUser->use_begin(), TheUser->use_end());
>>> +      }
>>> +    } while (!WL.empty());
>>>      }
>>
>> You should have an assertion somewhere that ensures that you find a DSNode
>> for the constant expression.
>>
> I'll add the above two optimizations you mentioned.
>
> Isn't is possible that the constant doesn't have a DSNode? In that
> case, it would be better to assert that a DSNodeHandle was found.

If it's a pointer, then it should probably have a DSNode unless it's a 
ConstantPointerNull.

The question as to whether have an assertion depends upon whether the 
code does the right thing if no DSNode is found for the ConstExpr.  If 
it does, then no assertion is needed.  If not, then you probably need an 
assert() somewhere.

-- John T.


>
> Matt
>
>> -- John T.
>>