[PATCH] [LinkModules] Intelligently merge triples that differ only in the minimum version number if the vendor is apple
Eric Christopher
echristo at gmail.com
Thu Feb 12 13:02:00 PST 2015
On Thu Feb 12 2015 at 12:58:12 PM Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > 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?
>
>
I think this sounds reasonable. It's the tack I've been taking when doing
this work. Numerous architectures also like to canonicalize various cpus in
their triples as well to enum values so I think this works out well.
-eric
> > > +
> > > + // 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"
> > >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150212/1f2e3e72/attachment.html>
More information about the llvm-commits
mailing list