[PATCH] D28088: MetadataLoader: simplify old type ref upgrade by removing a redundant set (NFC)

[Mehdi Amini via llvm-commits]

> On Dec 24, 2016, at 5:21 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> I don't think this is correct.  It's subtle and there should really be a testcase (I'm sorry I failed to provide one).
> 
> Note the behaviour change in the following scenario:
> - 1st translation unit has a fwd ref DICompositeType (!1).
> - 2nd translation unit has a full DICompositeType (!2).
> - 3rd translation unit has a full DICompositeType (!3).

Ohhh, in retrospective that’s obvious :)

We could do it with a single map, but on insertion overwriting FwdDecl with a Final one.

— 
Mehdi



> 
> In the code in ToT:
> - !1 gets inserted into FwdRefs.
> - !2 gets inserted into Final.
> - !3 gets ignored.
> 
> With your change, I believe:
> - !1 gets inserted into Final.
> - !2 gets ignored.
> - !3 gets ignored.
> 
> It's important that !2, the first non-fwd-ref DICompositeType, is the one that we keep (that "wins").
> 
> It shouldn't be too hard to make a testcase.  I suggest:
> - Three translation units.
> - Each one has a single DICompositeType referenced from a named MD node you make up.
> - uuid: "id"
> - name: "forward", "winner", and "loser" (for the three)
> 
> Should be easy to write a FileCheck against that.
> 
> 
>> On 2016-Dec-24, at 01:18, Mehdi AMINI via Phabricator <reviews at reviews.llvm.org> wrote:
>> 
>> mehdi_amini created this revision.
>> mehdi_amini added reviewers: pcc, dexonsmith.
>> mehdi_amini added a subscriber: llvm-commits.
>> 
>> I'm still trying to understand the logic, hopefully this is correct
>> (at least the validation is still passing).
>> 
>> 
>> https://reviews.llvm.org/D28088
>> 
>> Files:
>> llvm/lib/Bitcode/Reader/MetadataLoader.cpp
>> 
>> 
>> Index: llvm/lib/Bitcode/Reader/MetadataLoader.cpp
>> ===================================================================
>> --- llvm/lib/Bitcode/Reader/MetadataLoader.cpp
>> +++ llvm/lib/Bitcode/Reader/MetadataLoader.cpp
>> @@ -120,7 +120,6 @@
>>  struct {
>>    SmallDenseMap<MDString *, TempMDTuple, 1> Unknown;
>>    SmallDenseMap<MDString *, DICompositeType *, 1> Final;
>> -    SmallDenseMap<MDString *, DICompositeType *, 1> FwdDecls;
>>    SmallVector<std::pair<TrackingMDRef, TempMDTuple>, 1> Arrays;
>>  } OldTypeRefs;
>> 
>> @@ -246,11 +245,6 @@
>> 
>>  bool DidReplaceTypeRefs = false;
>> 
>> -  // Give up on finding a full definition for any forward decls that remain.
>> -  for (const auto &Ref : OldTypeRefs.FwdDecls)
>> -    OldTypeRefs.Final.insert(Ref);
>> -  OldTypeRefs.FwdDecls.clear();
>> -
>>  // Upgrade from old type ref arrays.  In strange cases, this could add to
>>  // OldTypeRefs.Unknown.
>>  for (const auto &Array : OldTypeRefs.Arrays) {
>> @@ -302,10 +296,7 @@
>> void BitcodeReaderMetadataList::addTypeRef(MDString &UUID,
>>                                           DICompositeType &CT) {
>>  assert(CT.getRawIdentifier() == &UUID && "Mismatched UUID");
>> -  if (CT.isForwardDecl())
>> -    OldTypeRefs.FwdDecls.insert(std::make_pair(&UUID, &CT));
>> -  else
>> -    OldTypeRefs.Final.insert(std::make_pair(&UUID, &CT));
>> +  OldTypeRefs.Final.insert(std::make_pair(&UUID, &CT));
>> }
>> 
>> Metadata *BitcodeReaderMetadataList::upgradeTypeRef(Metadata *MaybeUUID) {
>> 
>> 
>> <D28088.82432.patch>
>