<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Feb 12, 2015 at 9:47 AM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><br>
> On 2015-Feb-12, at 08:59, Tom Stellard <<a href="mailto:thomas.stellard@amd.com">thomas.stellard@amd.com</a>> wrote:<br>
><br>
><br>
><br>
><br>
> ================<br>
> Comment at: lib/Linker/LinkModules.cpp:1501-1505<br>
> @@ -1487,3 +1500,7 @@<br>
> DstM->getTargetTriple() + "'\n");<br>
> - }<br>
> +<br>
> + // If vendor is apple, pick the triple with the larger version number.<br>
> + if (SrcTriple.getVendor() == Triple::Apple)<br>
> + if (DstTriple.isOSVersionLT(SrcTriple))<br>
> + DstM->setTargetTriple(SrcM->getTargetTriple());<br>
><br>
> ----------------<br>
><br>
> I've run into this problem too when trying to link amdgcn triples with differing operating systems (e.g. amgcn-- and amdgcn--amdhsa).<br>
><br>
> 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.<br>
<br>
</span>I agree with Tom.<br>
<br>
A couple of other minor points below:<br>
<br>
> Index: lib/Linker/LinkModules.cpp<br>
> ===================================================================<br>
> --- lib/Linker/LinkModules.cpp<br>
> +++ lib/Linker/LinkModules.cpp<br>
> @@ -18,6 +18,7 @@<br>
> #include "llvm/ADT/SetVector.h"<br>
> #include "llvm/ADT/SmallString.h"<br>
> #include "llvm/ADT/Statistic.h"<br>
> +#include "llvm/ADT/Triple.h"<br>
> #include "llvm/IR/Constants.h"<br>
> #include "llvm/IR/DebugInfo.h"<br>
> #include "llvm/IR/DiagnosticInfo.h"<br>
> @@ -1457,6 +1458,16 @@<br>
> return HasErr;<br>
> }<br>
><br>
> +// This function returns true if the triples match.<br>
> +static bool triplesMatch(const Triple &T0, const Triple &T1) {<br>
> + if (T0.getVendor() != Triple::Apple)<br>
> + return T0.str() == T1.str();<br>
<br>
Shouldn't this just be `return T0 == T1`?<br>
<br></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
> +<br>
> + // If vendor is apple, ignore the version number.<br>
> + return T0.getArch() == T1.getArch() && T0.getSubArch() == T1.getSubArch() &&<br>
> + T0.getVendor() == T1.getVendor() && T0.getOS() == T1.getOS();<br>
<br>
More generally, I might invert the logic to make this more obviously<br>
extensible, e.g.:<br>
<br>
if (T0.getVender() == Triple::Apple)<br>
return T0.getArch() == T1.getArch() && ...;<br>
<br>
return T0 == T1;<br>
<br>
> +}<br>
> +<br>
> bool ModuleLinker::run() {<br>
> assert(DstM && "Null destination module");<br>
> assert(SrcM && "Null source module");<br>
> @@ -1466,26 +1477,32 @@<br>
> if (!DstM->getDataLayout() && SrcM->getDataLayout())<br>
> DstM->setDataLayout(SrcM->getDataLayout());<br>
><br>
> - // Copy the target triple from the source to dest if the dest's is empty.<br>
> - if (DstM->getTargetTriple().empty() && !SrcM->getTargetTriple().empty())<br>
> - DstM->setTargetTriple(SrcM->getTargetTriple());<br>
> -<br>
> if (SrcM->getDataLayout() && DstM->getDataLayout() &&<br>
> *SrcM->getDataLayout() != *DstM->getDataLayout()) {<br>
> emitWarning("Linking two modules of different data layouts: '" +<br>
> SrcM->getModuleIdentifier() + "' is '" +<br>
> SrcM->getDataLayoutStr() + "' whereas '" +<br>
> DstM->getModuleIdentifier() + "' is '" +<br>
> DstM->getDataLayoutStr() + "'\n");<br>
> }<br>
> - if (!SrcM->getTargetTriple().empty() &&<br>
> - DstM->getTargetTriple() != SrcM->getTargetTriple()) {<br>
> +<br>
> + // Copy the target triple from the source to dest if the dest's is empty.<br>
> + if (DstM->getTargetTriple().empty() && !SrcM->getTargetTriple().empty())<br>
> + DstM->setTargetTriple(SrcM->getTargetTriple());<br>
> +<br>
> + Triple SrcTriple(SrcM->getTargetTriple()), DstTriple(DstM->getTargetTriple());<br>
> +<br>
> + if (!SrcM->getTargetTriple().empty() && !triplesMatch(SrcTriple, DstTriple))<br>
> emitWarning("Linking two modules of different target triples: " +<br>
> SrcM->getModuleIdentifier() + "' is '" +<br>
> SrcM->getTargetTriple() + "' whereas '" +<br>
> DstM->getModuleIdentifier() + "' is '" +<br>
<span class="">> DstM->getTargetTriple() + "'\n");<br>
> - }<br>
> +<br>
> + // If vendor is apple, pick the triple with the larger version number.<br>
> + if (SrcTriple.getVendor() == Triple::Apple)<br>
> + if (DstTriple.isOSVersionLT(SrcTriple))<br>
> + DstM->setTargetTriple(SrcM->getTargetTriple());<br>
><br>
</span>> // Append the module inline asm string.<br>
> if (!SrcM->getModuleInlineAsm().empty()) {<br>
> Index: test/Linker/apple-version0.ll<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/Linker/apple-version0.ll<br>
<br>
I'd rename this `test/Linker/apple-version.ll`. More below.<br>
<br>
> @@ -0,0 +1,17 @@<br>
> +; RUN: llvm-link %s %p/apple-version1.ll -S -o - 2>%t.err | FileCheck %s -check-prefix=CHECK1<br>
<br>
I've usually used %S here instead of %p. I don't even see %p<br>
documented [1]. What's the difference?<br>
<br>
[1]: <a href="http://llvm.org/docs/TestingGuide.html#substitutions" target="_blank">http://llvm.org/docs/TestingGuide.html#substitutions</a></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
<br>
> +; RUN: (echo foo ;cat %t.err) | FileCheck --check-prefix=WARN1 %s<br>
<br>
Instead of `echo foo` you should use `FileCheck --allow-empty`.<br>
<br>
> +; RUN: llvm-link %s %p/apple-version2.ll -S -o - 2>%t.err | FileCheck %s -check-prefix=CHECK2<br>
> +; RUN: (echo foo ;cat %t.err) | FileCheck --check-prefix=WARN2 %s<br>
> +; RUN: llvm-link %s %p/apple-version3.ll -S -o /dev/null 2>%t.err<br>
> +; RUN: cat %t.err | FileCheck --check-prefix=WARN3 %s<br>
> +<br>
> +; Check that the triple that has the larger version number is chosen and no<br>
> +; warnings are issued when the triples differ only in version numbers.<br>
> +<br>
> +; CHECK1: target triple = "x86_64-apple-macosx10.10.0"<br>
> +; WARN1-NOT: WARNING<br>
> +; CHECK2: target triple = "x86_64-apple-macosx10.9.0"<br>
> +; WARN2-NOT: WARNING<br>
> +; WARN3: WARNING: Linking two modules of different target triples<br>
> +<br>
> +target triple = "x86_64-apple-macosx10.9.0"<br>
> Index: test/Linker/apple-version1.ll<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/Linker/apple-version1.ll<br>
> @@ -0,0 +1,4 @@<br>
> +; This file is used with apple-version0.ll<br>
> +; RUN: true<br>
<br>
Move to `test/Linker/Inputs/apple-version/1.ll` and skip the RUN<br>
(and the comment -- this way the paths are self-documenting).<br>
<br>
> +<br>
> +target triple = "x86_64-apple-macosx10.10.0"<br>
> Index: test/Linker/apple-version2.ll<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/Linker/apple-version2.ll<br>
<br>
`test/Linker/Inputs/apple-version/2.ll`<br>
<br>
> @@ -0,0 +1,4 @@<br>
> +; This file is used with apple-version0.ll<br>
> +; RUN: true<br>
> +<br>
> +target triple = "x86_64-apple-macosx10.8.0"<br>
> Index: test/Linker/apple-version3.ll<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/Linker/apple-version3.ll<br>
<br>
`test/Linker/Inputs/apple-version/3.ll`<br>
<br>
> @@ -0,0 +1,4 @@<br>
> +; This file is used with apple-version0.ll<br>
> +; RUN: true<br>
> +<br>
> +target triple = "x86-apple-macosx10.9.0"<br>
><br>
<br>
</blockquote></div><br></div></div>