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

[David Blaikie]

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)

- 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