[PATCH] [LinkModules] Intelligently merge triples that differ only in the minimum version number if the vendor is apple
Duncan P. N. Exon Smith
dexonsmith at apple.com
Thu Feb 12 12:58:10 PST 2015
> On 2015-Feb-12, at 12:05, Akira Hatanaka <ahatanak at gmail.com> wrote:
>
> On Thu, Feb 12, 2015 at 9:47 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
> > 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`?
>
>
> Reading the code now, I realize the code below this line should be fixed. My patch changes the current behavior, which is to issue a warning when the two triple strings don't match exactly. Triple's constructor parses different architecture strings to the same enum (e.g., "i386" and "i486" both map to Triple::x86), so in some cases tripleMatch would return true even when the strings don't match exactly.
Akira and I talked offline, and we both think `triplesMatch()` should
return true when the `Triple`s compare equal, even if the strings
differ. The backend only seems to look at the enums in the `Triple`
(except `SparcAsmParser::is64Bit()` and `SparcAsmBackend::is64Bit()`,
which both look like bugs).
Anyone disagree with this?
> > +
> > + // 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
>
> Judging from the code in TestRunner.py, It looks like they have the same meaning. I'm not sure why %S is documented, but %p isn't. I'll change my test cases to use %S to avoid confusion.
>
>
>
> > +; 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"
> >
More information about the llvm-commits
mailing list