[PATCH] [LinkModules] Intelligently merge triples that differ only in the minimum version number if the vendor is apple

[Duncan P. N. Exon Smith]

> On 2015-Feb-12, at 08:59, Tom Stellard <thomas.stellard at amd.com> wrote:
> 
> 
> 
> 
> ================
> Comment at: lib/Linker/LinkModules.cpp:1501-1505
> @@ -1487,3 +1500,7 @@
>                 DstM->getTargetTriple() + "'\n");
> -  }
> +
> +  // If vendor is apple, pick the triple with the larger version number.
> +  if (SrcTriple.getVendor() == Triple::Apple)
> +    if (DstTriple.isOSVersionLT(SrcTriple))
> +      DstM->setTargetTriple(SrcM->getTargetTriple());
> 
> ----------------
> 
> I've run into this problem too when trying to link amdgcn triples with differing operating systems  (e.g. amgcn-- and amdgcn--amdhsa).
> 
> Would it be possible to move this part into a separate function like you've done for triplesMatch(), so that it could be more easily updated for other targets.

I agree with Tom.

A couple of other minor points below:

> Index: lib/Linker/LinkModules.cpp
> ===================================================================
> --- lib/Linker/LinkModules.cpp
> +++ lib/Linker/LinkModules.cpp
> @@ -18,6 +18,7 @@
>  #include "llvm/ADT/SetVector.h"
>  #include "llvm/ADT/SmallString.h"
>  #include "llvm/ADT/Statistic.h"
> +#include "llvm/ADT/Triple.h"
>  #include "llvm/IR/Constants.h"
>  #include "llvm/IR/DebugInfo.h"
>  #include "llvm/IR/DiagnosticInfo.h"
> @@ -1457,6 +1458,16 @@
>    return HasErr;
>  }
>  
> +// This function returns true if the triples match.
> +static bool triplesMatch(const Triple &T0, const Triple &T1) {
> +  if (T0.getVendor() != Triple::Apple)
> +    return T0.str() == T1.str();

Shouldn't this just be `return T0 == T1`?

> +
> +  // If vendor is apple, ignore the version number.
> +  return T0.getArch() == T1.getArch() && T0.getSubArch() == T1.getSubArch() &&
> +      T0.getVendor() == T1.getVendor() && T0.getOS() == T1.getOS();

More generally, I might invert the logic to make this more obviously
extensible, e.g.:

    if (T0.getVender() == Triple::Apple)
      return T0.getArch() == T1.getArch() && ...;

    return T0 == T1;

> +}
> +
>  bool ModuleLinker::run() {
>    assert(DstM && "Null destination module");
>    assert(SrcM && "Null source module");
> @@ -1466,26 +1477,32 @@
>    if (!DstM->getDataLayout() && SrcM->getDataLayout())
>      DstM->setDataLayout(SrcM->getDataLayout());
>  
> -  // Copy the target triple from the source to dest if the dest's is empty.
> -  if (DstM->getTargetTriple().empty() && !SrcM->getTargetTriple().empty())
> -    DstM->setTargetTriple(SrcM->getTargetTriple());
> -
>    if (SrcM->getDataLayout() && DstM->getDataLayout() &&
>        *SrcM->getDataLayout() != *DstM->getDataLayout()) {
>      emitWarning("Linking two modules of different data layouts: '" +
>                  SrcM->getModuleIdentifier() + "' is '" +
>                  SrcM->getDataLayoutStr() + "' whereas '" +
>                  DstM->getModuleIdentifier() + "' is '" +
>                  DstM->getDataLayoutStr() + "'\n");
>    }
> -  if (!SrcM->getTargetTriple().empty() &&
> -      DstM->getTargetTriple() != SrcM->getTargetTriple()) {
> +
> +  // Copy the target triple from the source to dest if the dest's is empty.
> +  if (DstM->getTargetTriple().empty() && !SrcM->getTargetTriple().empty())
> +    DstM->setTargetTriple(SrcM->getTargetTriple());
> +
> +  Triple SrcTriple(SrcM->getTargetTriple()), DstTriple(DstM->getTargetTriple());
> +
> +  if (!SrcM->getTargetTriple().empty() && !triplesMatch(SrcTriple, DstTriple))
>      emitWarning("Linking two modules of different target triples: " +
>                  SrcM->getModuleIdentifier() + "' is '" +
>                  SrcM->getTargetTriple() + "' whereas '" +
>                  DstM->getModuleIdentifier() + "' is '" +
>                  DstM->getTargetTriple() + "'\n");
> -  }
> +
> +  // If vendor is apple, pick the triple with the larger version number.
> +  if (SrcTriple.getVendor() == Triple::Apple)
> +    if (DstTriple.isOSVersionLT(SrcTriple))
> +      DstM->setTargetTriple(SrcM->getTargetTriple());
>  
>    // Append the module inline asm string.
>    if (!SrcM->getModuleInlineAsm().empty()) {
> Index: test/Linker/apple-version0.ll
> ===================================================================
> --- /dev/null
> +++ test/Linker/apple-version0.ll

I'd rename this `test/Linker/apple-version.ll`.  More below.

> @@ -0,0 +1,17 @@
> +; RUN: llvm-link %s %p/apple-version1.ll -S -o - 2>%t.err | FileCheck %s -check-prefix=CHECK1

I've usually used %S here instead of %p.  I don't even see %p
documented [1].  What's the difference?

[1]: http://llvm.org/docs/TestingGuide.html#substitutions

> +; RUN: (echo foo ;cat %t.err) | FileCheck --check-prefix=WARN1 %s

Instead of `echo foo` you should use `FileCheck --allow-empty`.

> +; RUN: llvm-link %s %p/apple-version2.ll -S -o - 2>%t.err | FileCheck %s -check-prefix=CHECK2
> +; RUN: (echo foo ;cat %t.err) | FileCheck --check-prefix=WARN2 %s
> +; RUN: llvm-link %s %p/apple-version3.ll -S -o /dev/null 2>%t.err
> +; RUN: cat %t.err | FileCheck --check-prefix=WARN3 %s
> +
> +; Check that the triple that has the larger version number is chosen and no
> +; warnings are issued when the triples differ only in version numbers.
> +
> +; CHECK1: target triple = "x86_64-apple-macosx10.10.0"
> +; WARN1-NOT: WARNING
> +; CHECK2: target triple = "x86_64-apple-macosx10.9.0"
> +; WARN2-NOT: WARNING
> +; WARN3: WARNING: Linking two modules of different target triples
> +
> +target triple = "x86_64-apple-macosx10.9.0"
> Index: test/Linker/apple-version1.ll
> ===================================================================
> --- /dev/null
> +++ test/Linker/apple-version1.ll
> @@ -0,0 +1,4 @@
> +; This file is used with apple-version0.ll
> +; RUN: true

Move to `test/Linker/Inputs/apple-version/1.ll` and skip the RUN
(and the comment -- this way the paths are self-documenting).

> +
> +target triple = "x86_64-apple-macosx10.10.0"
> Index: test/Linker/apple-version2.ll
> ===================================================================
> --- /dev/null
> +++ test/Linker/apple-version2.ll

`test/Linker/Inputs/apple-version/2.ll`

> @@ -0,0 +1,4 @@
> +; This file is used with apple-version0.ll
> +; RUN: true
> +
> +target triple = "x86_64-apple-macosx10.8.0"
> Index: test/Linker/apple-version3.ll
> ===================================================================
> --- /dev/null
> +++ test/Linker/apple-version3.ll

`test/Linker/Inputs/apple-version/3.ll`

> @@ -0,0 +1,4 @@
> +; This file is used with apple-version0.ll
> +; RUN: true
> +
> +target triple = "x86-apple-macosx10.9.0"
>