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

[Jeffrey Yasskin]

On Thu, Mar 4, 2010 at 4:40 PM, Chris Lattner <clattner at apple.com> wrote:
>
> 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.

Another way to avoid the lock would be to contextify this. It probably
wouldn't be that hard; just require an extra parameter everywhere it's
used.