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

[Duncan P. N. Exon Smith via llvm-commits]

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).

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>