Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Triple] Ignore the vendor field for MinGW, wrt LTO/IR compatibility #122801

Merged
merged 1 commit into from
Jan 14, 2025
Merged

[Triple] Ignore the vendor field for MinGW, wrt LTO/IR compatibility #122801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions llvm/lib/TargetParser/Triple.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2024,6 +2024,10 @@ bool Triple::isLittleEndian() const {
}

bool Triple::isCompatibleWith(const Triple &Other) const {
// On MinGW, C code is usually built with a "w64" vendor, while Rust
// often uses a "pc" vendor.
Comment on lines +2027 to +2028
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is uwp-windows-gnu compatible the other two mentioned variants? I think so but wanted to double-check.
Not sure if it's even a real triple in LLVM. Rust has it, but the GNU env is unmaintained: https://doc.rust-lang.org/rustc/platform-support.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for pointing it out!

On an LTO/IR level, there's nothing there that would be incompatible. Of course, on functional level, code that doesn't target UWP may or may not work correct if code is linked in that isn't written with a UWP target in mind (but this issue is the same even if not involving LTO).

Also, thanks for pointing out the Rust use of uwp for the vendor field here; going that way would help using Clang config files for the UWP targets in llvm-mingw. (Currently we use the environment name mingw32uwp, but that gets normalized into the same windows-gnu as non-UWP.)

Not sure if it's even a real triple in LLVM.

The vendor field is pretty much freeform and not really used by anything as far as I'm aware. Sure, there are a couple of known ones and an enum for them, but neither w64 nor uwp map to any of the known enum values: https://github.com/llvm/llvm-project/blob/llvmorg-19.1.6/llvm/lib/TargetParser/Triple.cpp#L627-L644

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't sure if there are any differences other than headers and libraries. We're good then.

bool IgnoreVendor = isWindowsGNUEnvironment();

// ARM and Thumb triples are compatible, if subarch, vendor and OS match.
if ((getArch() == Triple::thumb && Other.getArch() == Triple::arm) ||
(getArch() == Triple::arm && Other.getArch() == Triple::thumb) ||
Expand All @@ -2034,17 +2038,24 @@ bool Triple::isCompatibleWith(const Triple &Other) const {
getVendor() == Other.getVendor() && getOS() == Other.getOS();
else
return getSubArch() == Other.getSubArch() &&
getVendor() == Other.getVendor() && getOS() == Other.getOS() &&
(getVendor() == Other.getVendor() || IgnoreVendor) &&
getOS() == Other.getOS() &&
getEnvironment() == Other.getEnvironment() &&
getObjectFormat() == Other.getObjectFormat();
}

// If vendor is apple, ignore the version number.
// If vendor is apple, ignore the version number (the environment field)
// and the object format.
if (getVendor() == Triple::Apple)
return getArch() == Other.getArch() && getSubArch() == Other.getSubArch() &&
getVendor() == Other.getVendor() && getOS() == Other.getOS();

return *this == Other;
(getVendor() == Other.getVendor() || IgnoreVendor) &&
getOS() == Other.getOS();

return getArch() == Other.getArch() && getSubArch() == Other.getSubArch() &&
(getVendor() == Other.getVendor() || IgnoreVendor) &&
getOS() == Other.getOS() &&
getEnvironment() == Other.getEnvironment() &&
getObjectFormat() == Other.getObjectFormat();
}

std::string Triple::merge(const Triple &Other) const {
Expand Down
38 changes: 38 additions & 0 deletions llvm/unittests/TargetParser/TripleTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//

#include "llvm/TargetParser/Triple.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/VersionTuple.h"
#include "gtest/gtest.h"

Expand Down Expand Up @@ -2737,4 +2738,41 @@ TEST(TripleTest, DXILNormaizeWithVersion) {
EXPECT_EQ("dxilv1.0-pc-shadermodel5.0-compute",
Triple::normalize("dxil-shadermodel5.0-pc-compute"));
}

TEST(TripleTest, isCompatibleWith) {
struct {
const char *A;
const char *B;
bool Result;
} Cases[] = {
{"armv7-linux-gnueabihf", "thumbv7-linux-gnueabihf", true},
{"armv4-none-unknown-eabi", "thumbv6-unknown-linux-gnueabihf", false},
{"x86_64-apple-macosx10.9.0", "x86_64-apple-macosx10.10.0", true},
{"x86_64-apple-macosx10.9.0", "i386-apple-macosx10.9.0", false},
{"x86_64-apple-macosx10.9.0", "x86_64h-apple-macosx10.9.0", true},
{"x86_64-unknown-linux-gnu", "x86_64-unknown-linux-gnu", true},
{"x86_64-unknown-linux-gnu", "i386-unknown-linux-gnu", false},
{"x86_64-unknown-linux-gnu", "x86_64h-unknown-linux-gnu", true},
{"x86_64-pc-windows-gnu", "x86_64-pc-windows-msvc", false},
{"x86_64-pc-windows-msvc", "x86_64-pc-windows-msvc-elf", false},
{"i686-w64-windows-gnu", "i386-w64-windows-gnu", true},
{"x86_64-w64-windows-gnu", "x86_64-pc-windows-gnu", true},
{"armv7-w64-windows-gnu", "thumbv7-pc-windows-gnu", true},
};

auto DoTest = [](const char *A, const char *B,
bool Result) -> testing::AssertionResult {
if (Triple(A).isCompatibleWith(Triple(B)) != Result) {
return testing::AssertionFailure()
<< llvm::formatv("Triple {0} and {1} were expected to be {2}", A,
B, Result ? "compatible" : "incompatible");
}
return testing::AssertionSuccess();
};
for (const auto &C : Cases) {
EXPECT_TRUE(DoTest(C.A, C.B, C.Result));
// Test that the comparison is commutative.
EXPECT_TRUE(DoTest(C.B, C.A, C.Result));
}
}
} // end anonymous namespace
Loading