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.

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

Enable aarch64-amazon-linux triple #8228

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

sebsto
Copy link

@sebsto sebsto commented Feb 17, 2024

add aarch64-amazon-linux triplet to allow compilation for Amazon Linux 2023 on Graviton CPU

This should address #8227

@MaxDesiatov MaxDesiatov changed the title add aarch64-amazon-linux triplet Enable aarch64-amazon-linux triple Feb 19, 2024
@MaxDesiatov
Copy link

Can this PR also be submitted to the upstream LLVM repository?

@sebsto
Copy link
Author

sebsto commented Feb 19, 2024

@MaxDesiatov thank you for the quick review.
Here is the upstream PR llvm#82232

Copy link

@al45tair al45tair left a comment

Choose a reason for hiding this comment

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

Not sure how we square this with

Please don't add more elements to *Triples.

in the comment block above the Triples arrays.

It seems like we'll have to add this or things won't work, though really the problem is things calling the compiler with an incomplete triple.

@sebsto
Copy link
Author

sebsto commented Feb 19, 2024

@ALstair good catch about the comment above :-)
Do you know what part of the build scripts invoke clang with incorrect triples ? I can try to modify the toolchain build scripts instead of clang itself

@al45tair
Copy link

Do you know what part of the build scripts invoke clang with incorrect triples ? I can try to modify the toolchain build scripts instead of clang itself

Looking at #8227 I wonder whether there's a problem with the front-end figuring out what the triple should be in the first place, as CMake failed during a link step when trying to do

bin/clang   CMakeFiles/cmTC_a5c7b.dir/testCCompiler.c.o -o cmTC_a5c7b

and if adding that triple to the list fixes it, that suggests that clang wasn't sure what triple it was supposed to be using in the first place for some reason. I don't know OTOH how it works out what the default triple should be, but I think that's probably where we should be looking for the problem here.

@al45tair
Copy link

Looks like maybe llvm::sys::getDefaultTargetTriple()?

@al45tair
Copy link

Which seems to be set from LLVM_DEFAULT_TARGET_TRIPLE.

@al45tair
Copy link

OK, so I think what happens is LLVM_DEFAULT_TARGET_TRIPLE is set from LLVM_HOST_TRIPLE, which is in turn set from LLVM_INFERRED_HOST_TRIPLE, which is initialised by llvm/cmake/modules/GetHostTriple.cmake, which, for things that are not in a small list of exceptions, runs a copy of config.guess.

Presumably this is generating the wrong result (I think it probably returns aarch64-unknown-linux-gnu), and I assume that the Triples arrays are in part an attempt to work around that problem.

My guess is that you should update GetHostTriple.cmake to work out the proper triple, since that would result in LLVM_DEFAULT_TARGET_TRIPLE being correct, which then means the compiler won't need to search. It looks like there was an intent to fix that by having the build process use the -dumpmachine option to clang instead of config.guess. See https://reviews.llvm.org/D109727, https://reviews.llvm.org/D109837, a077271.

@MaxDesiatov
Copy link

Presumably this is generating the wrong result (I think it probably returns aarch64-unknown-linux-gnu), and I assume that the Triples arrays are in part an attempt to work around that problem.

I was also seeing this issue when trying to build on Alpine Linux. It incorrectly inferred aarch64-unknown-linux-musl when I needed aarch64-alpine-linux-musl for things to work "the Alpine way".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants