[patch][pr19848] Don't create new comdats in CodeGen

[David Majnemer]

Why do you create the explicit comdat group in ParseValueSymbolTable
instead of when you detect the implicit comdat in ParseModule?

On Thu, Jan 8, 2015 at 2:12 PM, Rafael Espíndola <rafael.espindola at gmail.com
> wrote:

> Updated patch attached. Just waiting on the review of the clang one.
>
> On 8 January 2015 at 13:14, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
> >
> >> On 2015-Jan-08, at 08:33, Rafael Espíndola <rafael.espindola at gmail.com>
> wrote:
> >>
> >> On 8 January 2015 at 03:30, David Majnemer <david.majnemer at gmail.com>
> wrote:
> >>> I really like this change.  However, don't we need to upgrade existing
> >>> bitcode by inserting stuff into newly made comdats?
> >>
> >> I guess so.
> >>
> >> It was not initially clear to me that it would be worth it. Old
> >> bitcode files would still work, they would just produce .o files that
> >> would not be as optimized by ELF/COFF linkers.
> >>
> >> But the feature is common enough that we should probably upgrade. The
> >> attached patch does that.
> >>
> >> Cheers,
> >> Rafael
> >> <t.patch>
> >
> > Aside from a comment nit, this LGTM.  I agree that upgrading makes sense.
> >
> >> diff --git a/lib/Bitcode/Reader/BitcodeReader.cpp
> b/lib/Bitcode/Reader/BitcodeReader.cpp
> >> index 7c7eebd..c9c08c0 100644
> >> --- a/lib/Bitcode/Reader/BitcodeReader.cpp
> >> +++ b/lib/Bitcode/Reader/BitcodeReader.cpp
> >> @@ -100,19 +100,31 @@ static bool ConvertToString(ArrayRef<uint64_t>
> Record, unsigned Idx,
> >>    return false;
> >>  }
> >>
> >> +static bool hasImplicitComdat(size_t Val) {
> >> +  switch (Val) {
> >> +  default:
> >> +    return false;
> >> +  case 1:  // old WeakAnyLinkage
> >> +  case 4:  // old LinkOnceAnyLinkage
> >> +  case 10: // old WeakODRLinkage
> >> +  case 11: // old LinkOnceODRLinkage
> >> +    return true;
> >> +  }
> >> +}
> >> +
> >>  static GlobalValue::LinkageTypes getDecodedLinkage(unsigned Val) {
> >>    switch (Val) {
> >>    default: // Map unknown/new linkages to external
> >>    case 0:
> >>      return GlobalValue::ExternalLinkage;
> >>    case 1:
> >> -    return GlobalValue::WeakAnyLinkage;
> >> +    return GlobalValue::WeakAnyLinkage; // Old value
> >
> > I think `// Implicit comdat` is better than `// Old value` here.  Or
> > maybe `// Old value with implicit comdat`?
> >
> >>    case 2:
> >>      return GlobalValue::AppendingLinkage;
> >>    case 3:
> >>      return GlobalValue::InternalLinkage;
> >>    case 4:
> >> -    return GlobalValue::LinkOnceAnyLinkage;
> >> +    return GlobalValue::LinkOnceAnyLinkage; // Old value
> >>    case 5:
> >>      return GlobalValue::ExternalLinkage; // Obsolete DLLImportLinkage
> >>    case 6:
> >> @@ -124,9 +136,9 @@ static GlobalValue::LinkageTypes
> getDecodedLinkage(unsigned Val) {
> >>    case 9:
> >>      return GlobalValue::PrivateLinkage;
> >>    case 10:
> >> -    return GlobalValue::WeakODRLinkage;
> >> +    return GlobalValue::WeakODRLinkage; // Old value
> >>    case 11:
> >> -    return GlobalValue::LinkOnceODRLinkage;
> >> +    return GlobalValue::LinkOnceODRLinkage; // Old value
> >>    case 12:
> >>      return GlobalValue::AvailableExternallyLinkage;
> >>    case 13:
> >>
> >> @@ -135,6 +147,14 @@ static GlobalValue::LinkageTypes
> getDecodedLinkage(unsigned Val) {
> >>      return GlobalValue::PrivateLinkage; // Obsolete
> LinkerPrivateWeakLinkage
> >>    case 15:
> >>      return GlobalValue::ExternalLinkage; // Obsolete
> LinkOnceODRAutoHideLinkage
> >> +  case 16:
> >> +    return GlobalValue::WeakAnyLinkage;
> >
> > Or maybe you should just move it down here:
> >
> >     case 1: // Old value with implicit comdat
> >     case 16:
> >         return GlobalValue::WeakAnyLinkage;
> >
> >> +  case 17:
> >> +    return GlobalValue::WeakODRLinkage;
> >> +  case 18:
> >> +    return GlobalValue::LinkOnceAnyLinkage;
> >> +  case 19:
> >> +    return GlobalValue::LinkOnceODRLinkage;
> >>    }
> >>  }
> >>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150112/0d5e0654/attachment.html>