[PATCH] Alternative way of representing comdats

[David Majnemer]

================
Comment at: docs/LangRef.rst:732
@@ +731,3 @@
+specify this key will only end up in the final object file if the linker chooses
+that key over some other key.
+
----------------
Rafael Ávila de Espíndola wrote:
> Other set of objects/sections with that key.
> 
> "the linker" in here is a bit ambiguous. Are we talking about the system linker or lib/Linker. Since we produce relocatable objects, it is probably better to state this in terms of what we do: put the global objects in a section with that comdat.
> 
The objects aren't in the same section.  In COFF, they are in different sections which are associated.

================
Comment at: docs/LangRef.rst:734
@@ +733,3 @@
+
+Note that the Mach-O platform doesn't support COMDATs and ELF only supports
+``any`` as a selection kind.
----------------
Rafael Ávila de Espíndola wrote:
> Move this after the part the defines what the selection kind is maybe?
Done.

================
Comment at: docs/LangRef.rst:761
@@ +760,3 @@
+
+Note that not all targets support COMDATs and some may only support ``any`` as a
+selection kind.
----------------
Rafael Ávila de Espíndola wrote:
> This paragraph is now redundant.
Done.

================
Comment at: docs/LangRef.rst:763
@@ +762,3 @@
+selection kind.
+
+Here is an example of a COMDAT group where a function will only be selected if
----------------
Rafael Ávila de Espíndola wrote:
> There are still some things should probably be mentioned:
> 
> * We split "IR level sections" into multiple sections in the final .o since in object files you can't have a section that is part in one comdat and part in another.
> * Explain how we map mulitple sections with the same comdat to using associative comdts on COFF. Otherwise someone is sure to look at the list and think associative is missing :-)
> * Alias are placed in the same comdat the aliasee computes to, if any.
I did the last two, I don't quite understand the first one.

================
Comment at: include/llvm/IR/GlobalValue.h:115
@@ +114,3 @@
+  bool hasComdat() const { return getComdat() != nullptr; }
+  Comdat *getComdat() {
+    return const_cast<Comdat *>(
----------------
Rafael Ávila de Espíndola wrote:
> This looks odd. You probably want the const version to forward to the non-const:
> 
> return const_cast<GlobalValue*>(this)->getComdat();
> 
Done.

================
Comment at: include/llvm/Support/GCOV.h:369
@@ +368,3 @@
+          BranchesExec(0), BranchesTaken(0) {}
+    GCOVCoverage(StringRef Name)
+        : Name(Name), LogicalLines(0), LinesExec(0), Branches(0),
----------------
Rafael Ávila de Espíndola wrote:
> unrelated change?
Done.

================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:252
@@ -242,4 +251,3 @@
       !Kind.isCommon()) {
-    const char *Prefix;
-    Prefix = getSectionPrefixForGlobal(Kind);
+    StringRef Prefix = getSectionPrefixForGlobal(Kind);
 
----------------
Rafael Ávila de Espíndola wrote:
> Nice cleanup. You can commit this now if you want.
Done in r211601.

================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:844
@@ -781,1 +843,3 @@
+  if ((GV->isWeakForLinker() || EmitUniquedSection || GV->hasComdat()) &&
+      !Kind.isCommon()) {
     const char *Name = getCOFFSectionNameForUniqueGlobal(Kind);
----------------
Rafael Ávila de Espíndola wrote:
> Reminds me: we should probably reject GlobalObject with common linkage and an explicit comdat in the verifier.
Done.

================
Comment at: lib/Linker/LinkModules.cpp:542
@@ -536,1 +541,3 @@
 
+static GlobalValue *getComdatLeader(Module *M, StringRef ComdatName) {
+  GlobalValue *GV = M->getNamedValue(ComdatName);
----------------
Rafael Ávila de Espíndola wrote:
> Not sure I follow. Is this assuming that the comdat name is related to the name of a global? That may be a common use but is not in any way required by the semantics.
> 
The COMDAT name **is** related to the name of the global when the selection kind isn't `any`.

================
Comment at: lib/Linker/LinkModules.cpp:558
@@ +557,3 @@
+  if (DstC && SrcC) {
+    Comdat::SelectionKind DSK = DstC->getSelectionKind();
+    Comdat::SelectionKind SSK = SrcC->getSelectionKind();
----------------
Rafael Ávila de Espíndola wrote:
> I assume this logic matches what the COFF linker does? Can you extract it to a helper like
> 
> bool computeResultingSeelectionKind(Comdat::SelectionKind &Result, Comdat::SelectionKind &Src, Comdat::SelectionKind &Dst) ...
> 
> and put a comment saying where the merging logic comes from.
Done.

================
Comment at: lib/Linker/LinkModules.cpp:583
@@ +582,3 @@
+    case Comdat::SelectionKind::Newest:
+      // Go with src
+      LinkFromSrc = true;
----------------
Rafael Ávila de Espíndola wrote:
> This is fairly arbitrary for Newest. We should probably error or actually check the bitcode timestamp, no?
Done.

================
Comment at: lib/Linker/LinkModules.cpp:595
@@ +594,3 @@
+
+      const auto SrcGV = dyn_cast<GlobalVariable>(SrcGVLeader);
+      const auto DstGV = dyn_cast<GlobalVariable>(DstGVLeader);
----------------
Rafael Ávila de Espíndola wrote:
> "auto *" instead of "auto" is probably better since it makes it obvious it is a pointer.
Done.

================
Comment at: lib/Linker/LinkModules.cpp:626
@@ +625,3 @@
+        return emitError("Linking COMDATs named '" + ComdatName +
+                         "': ExactMatch requries GlobalVariable");
+      }
----------------
Rafael Ávila de Espíndola wrote:
> Not just ExactMatch.
Done.

================
Comment at: lib/Linker/LinkModules.cpp:865
@@ +864,3 @@
+  bool LinkFromSrc = false;
+  Comdat *DC = nullptr;
+  if (const Comdat *SC = SGV->getComdat()) {
----------------
Rafael Ávila de Espíndola wrote:
> BTW, do we produce as error if we have
> 
> module1:
> 
> @a = ... comdat $c1, largest
> 
> module2
> 
> @b = ... comdat $c1, newest
> 
> that is, we have conflicting comdat selection kinds, but the comdats never show up in the corresponding global objects.
> 
There is code for this:
    return emitError("Linking COMDATs named '" + ComdatName +
                     "': invalid selection kinds!");

http://reviews.llvm.org/D4178