[llvm] r202417 - Revert r201751 and solve the const problem a different way - by

[Eric Christopher]

On Thu, Feb 27, 2014 at 11:02 AM, David Blaikie <dblaikie at gmail.com> wrote:
> On Thu, Feb 27, 2014 at 10:51 AM, Eric Christopher <echristo at gmail.com> wrote:
>>>> +  if (!Size) {
>>>> +    const SmallVectorImpl<DIEAbbrevData> &AbbrevData = Abbrev.getData();
>>>> +    for (unsigned i = 0, N = Values.size(); i < N; ++i)
>>>> +      Size += Values[i]->SizeOf(AP, AbbrevData[i].getForm());
>>>> +  }
>>>
>>> Should we add an assert here to make sure the Size isn't different
>>> from the cached size? (should we ever end up with a situation in which
>>> a caller computes the size, then adds things, then retrieves the size
>>> again - thus getting the incorrect, cached, size not the current size)
>>> - it would mean debug builds always compute the size, never relying on
>>> the cache, but release builds rely on it and don't recompute it.
>>
>> We could add ... right. Um, sure, it seems reasonable :)
>>
>>>>  void DwarfUnit::addBlock(DIE *Die, dwarf::Attribute Attribute, DIELoc *Loc) {
>>>> -  Loc->setSize(Loc->ComputeSize(Asm));
>>>> +  Loc->ComputeSize(Asm);
>>>>    DIELocs.push_back(Loc); // Memoize so we can call the destructor later on.
>>>>    Die->addValue(Attribute, Loc->BestForm(DD->getDwarfVersion()), Loc);
>>>>  }
>>>>
>>>>  void DwarfUnit::addBlock(DIE *Die, dwarf::Attribute Attribute,
>>>>                           DIEBlock *Block) {
>>>> -  Block->setSize(Block->ComputeSize(Asm));
>>>> +  Block->ComputeSize(Asm);
>>>
>>> Do we need to call ComputeSize here at all, then?
>>>
>>
>> Interesting question - the brief answer is "yes" but only because
>> everything accesses by grabbing the Size member rather than calling
>> ComputeSize.
>
> Oh. That's interesting... yeah - clearly I've not looked at this
> closely enough.
>
>>
>>> (is "getSize" a better name for this function, rather than
>>> ComputeSize? I dunno - haven't looked at the hierarchy and callers,
>>> etc)
>>
>> Totally. Yes.
>
> Well, if it's really for computing the size (& everything that /uses/
> the size just accesses the member directly) then perhaps my
> suggestions don't make sense. Instead I guess we could just have
> ComputeSize be void returning (since no one's using its result). I
> wonder if it needs the caching behavior at all? Maybe it can just be
> "UpdateSize" and the few cases where that causes a redundant
> recomputation can just be acceptable losses? I'm not sure how
> pervasive your change makes such extra recomputation...
>
> blah blah - don't mind me, clearly don't know the code well enough,
> probably only worth bantering about over lunch or something.
>
> (extreme proposal: have some way to initialize a block with all its
> data and no mutation - then just compute the size once with no
> possibility of it getting out of date, recomputation, caching, etc)
>

That's kinda what's going on. ComputeSize could really be called
"Finalize" since that's what's happening there.

-eric

> - David
>
>>
>> -eric
>>
>>>
>>>>    DIEBlocks.push_back(Block); // Memoize so we can call the destructor later on.
>>>>    Die->addValue(Attribute, Block->BestForm(), Block);
>>>>  }
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits