[llvm-commits] [llvm] r97759 - /llvm/trunk/lib/CodeGen/PseudoSourceValue.cpp

[Chris Lattner]

On Mar 4, 2010, at 2:33 PM, Jeffrey Yasskin wrote:

>>> +namespace {
>>> +struct PSVGlobalsTy {
>>> +  // PseudoSourceValues are immutable so don't need locking.
>>> +  const PseudoSourceValue PSVs[4];
>>> +  sys::Mutex Lock;  // Guards FSValues, but not the values inside it.
>>> +  std::map<int, const PseudoSourceValue *> FSValues;
>>> +
>>> +  PSVGlobalsTy() : PSVs() {}
>>> +  ~PSVGlobalsTy() {
>>> +    for (std::map<int, const PseudoSourceValue *>::iterator
>>> +           I = FSValues.begin(), E = FSValues.end(); I != E; ++I) {
>>> +      delete I->second;
>> 
>> Why does this need an std::map?  Aren't the keys dense integers?  Why not directly index into the array, allowing elimination of the lock?
> 
> PseudoSourceValue::getFixedStack lets the user pass in an arbitrary
> int. It could be that these tend to be dense, allowing us to use a
> vector for FSValues instead of the map, but the vector would still
> have to be growable to accommodate larger integers than it's seen yet,
> which would still require a lock. Since I don't know how this is used,
> I just rearranged it instead of trying to change the data structure or
> move it into the Context.

Ah, I see.  Dan, what do you think? a lock + std::map seems expensive.