Triple::normalize: Use none as OS for XX-none-ABI

[wzssyqa]

When parsing a 3-component triple, after we determine Arch and Env, if the middle component is "none", treat it as OS instead of Vendor.

See: https://discourse.llvm.org/t/rfc-baremetal-target-triple-normalization/78524
Fixes: #89582.

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-libcxx

Author: YunQiang Su (wzssyqa)

Changes

If the middle component of a 3-component triple fails to parse as known Arch/Vendor/OS/Env, it will fallback as Vendor. While for some cases, we may wish to recognize it as OS, such as arm64-none-elf.

In this patch, we will set OS as none, if:
1) Arch is found;
2) Env is found;
3) OS is not found and thus is set as empty;
4) Vendor is not found while is set as "none",
we will swap Component[2] and Component[3].

Fixes: #89582.

---

Full diff: https://github.com/llvm/llvm-project/pull/89638.diff

2 Files Affected:

• (modified) libcxx/utils/ci/run-buildbot (+1-1)
• (modified) llvm/lib/TargetParser/Triple.cpp (+7)

diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
index 60307a7d4f350a..3523a29e4f4613 100755
--- a/libcxx/utils/ci/run-buildbot
+++ b/libcxx/utils/ci/run-buildbot
@@ -217,7 +217,7 @@ function test-armv7m-picolibc() {
         "${@}"
 
     ${NINJA} -vC "${BUILD_DIR}/compiler-rt" install
-    mv "${BUILD_DIR}/install/lib/armv7m-none-unknown-eabi"/* "${BUILD_DIR}/install/lib"
+    mv "${BUILD_DIR}/install/lib/armv7m-unknown-none-eabi"/* "${BUILD_DIR}/install/lib"
 
     check-runtimes
 }
diff --git a/llvm/lib/TargetParser/Triple.cpp b/llvm/lib/TargetParser/Triple.cpp
index 77fdf31d4865c0..07f3df4145dad4 100644
--- a/llvm/lib/TargetParser/Triple.cpp
+++ b/llvm/lib/TargetParser/Triple.cpp
@@ -1149,6 +1149,13 @@ std::string Triple::normalize(StringRef Str) {
     }
   }
 
+  // For 3-component triples, the middle component is used to set Vendor;
+  // while if it is "none", we'd prefer to set OS.
+  // This is for some baremetal cases, such as "arm-none-elf".
+  if (Found[0] && !Found[1] && !Found[2] && Found[3] &&
+      Components[1].equals("none") && Components[2].empty())
+    std::swap(Components[1], Components[2]);
+
   // Replace empty components with "unknown" value.
   for (StringRef &C : Components)
     if (C.empty())

[wzssyqa]

@peterwaller-arm Ohh, there is so many -none-unknown- in current code. I guess it may be widely used.
Do we really want to change all of them?

[wzssyqa]

@llvm/pr-subscribers-backend-arm @llvm/pr-subscribers-backend-aarch64

[peterwaller-arm]

I have tested building a toolchain using aarch64-none-elf, and the problems described in my comment on #89234 (comment) are resolved. The only change I had to make was to update some references to aarch64-none-elf to reference aarch64-unknown-none-elf, reflecting the change in compiler-rt install paths introduced in #89234.

I concur with Sander that it seems prudent to have the functional change in its own commit, in case it requires further revert flip-flop.

[DavidSpickett]

Ohh, there is so many -none-unknown- in current code. I guess it may be widely used. Do we really want to change all of them?

In current test cases, which were likely generated using clang. So they are likely not relying on that exact form of the triple, it's just what clang put out at the time. So they don't prove anything either way, unless they fail after this change of course.

Thank you Peter for testing this PR, I've also asked another downstream team to test their bare metal builds with this. That'll be our best indication for now if this makes sense.

[peterwaller-arm]

I've just posted an RFC to discourse about how to handle triple normalization: https://discourse.llvm.org/t/rfc-baremetal-target-triple-normalization/78524

[wzssyqa]

I've just posted an RFC to discourse about how to handle triple normalization: https://discourse.llvm.org/t/rfc-baremetal-target-triple-normalization/78524

We don't need to revert #89234
It is easy to fix Triple::normalize once you decide how to normalize your triples.
I will help to do so.

[MaskRay]

The description is a bit difficult to parse. I'll suggest that using some grammar checker to fix the description.
But thanks for putting up the patch. I think it is desired.

https://discourse.llvm.org/t/rfc-baremetal-target-triple-normalization/78524/8

The number of none-unknown test changes actually looks manageable.
The worst case is that we notice some backward compatibility story that we don’t account for today,
and we add temporary workarounds for clang --target=aarch64-none-elf to probe lib/aarch64-none-unknown-elf (old LLVM_ENABLE_PER_TARGET_RUNTIME_DIR hierarchy)
beside lib/aarch64-unknown-none-elf.

[peterwaller-arm]

Thanks @wzssyqa again for implementing this and splitting bits out per Sander's suggestion. I propose we merge these changes tomorrow, on the understanding that each of the two patches (this and #90313) are passing tests. It looks to me like you're working towards that goal.

What I see in CI for this PR is LLVM-Unit :: TargetParser/./TargetParserTests/TripleTest/Normalization is currently failing.

Is it possible to have two patches such that they're both green? My expectation is that the majority of the triple changes in the source are unrelated to normalization and would work the same before and after the functional changes to triple normalization. (And that those tests which are impacted by changes to triple normalization should of course change in the same commit to keep things green).

[wzssyqa]

@peterwaller-arm It seems OK now. Let's wait the result of CI.

[MaskRay]

When we parse 3-components triples, if the Arch and Env have been parsed successfully, we have to make a choice between Vendor and OS for the unrecoginzed component. Noramlly it is the middle one.

In the current code, Vendor is choosed, and then OS is fallbacked to unknown. It is OK for most cases. But if the unrecoginzed component is none, it is expected to be OS instead of Vendor.

Suggested:

When parsing a 3-component triple, after we determine Arch and Env, if the middle component is "none", treat it as OS instead of Vendor.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[DavidSpickett]

I'm checking the picolib libcxx build right now so please hold off on landing this.

[peterwaller-arm]

Thanks for doing this and waiting for RFC comments.

I think this can be merged, the one thing which needs fixing is removing the change I've commented on so that the picolibc builder can succeed (it uses a version of clang on the builder, which @DavidSpickett will update at the right time).

[DavidSpickett]

Also this change should be release noted with a tip that build scripts may want to change to asking clang for the triple instead.

[peterwaller-arm]

LGTM, thanks for supplying a quick fix and iterating this, and thanks for waiting for input on the RFC.

[DavidSpickett]

libcxx is sorted, and I will write up the release note given that I suggested it and have been fixing some builds myself.

[MaskRay]

#90734 provided a release note.

The armv7m-none-eabi example can probably be copied to improve the commit message/description.

[DavidSpickett]

@wzssyqa thanks for working with us on this. Bare metal is always a wild card but I think in the end we'll all have cleaner builds because of these changes.