Page MenuHomePhabricator

[CMake] Only set LLVM_DEFAULT_TARGET_TRIPLE to LLVM_HOST_TRIPLE when native target is enabled
ClosedPublic

Authored by python3kgae on Sep 30 2022, 10:10 AM.

Details

Summary

This is for case when native target like X86 is not in LLVM_TARGETS_TO_BUILD.
Right now LLVM_DEFAULT_TARGET_TRIPLE is set to LLVM_HOST_TRIPLE even when native target is not enabled,
As a result, many lit tests will fail because default_triple is set for lit test but not enabled when build LLVM.

Diff Detail

Event Timeline

python3kgae created this revision.Sep 30 2022, 10:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 10:10 AM
python3kgae requested review of this revision.Sep 30 2022, 10:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 10:10 AM

Sorry for the slow review.

Does this mean that LLVM_DEFAULT_TARGET_TRIPLE will now be empty when the native target isn't enabled (since LLVM_DEFAULT_TARGET_TRIPLE_default won't be set)? Does that cause any problems?

Sorry for the slow review.

Does this mean that LLVM_DEFAULT_TARGET_TRIPLE will now be empty when the native target isn't enabled (since LLVM_DEFAULT_TARGET_TRIPLE_default won't be set)? Does that cause any problems?

Thanks for reviewing.
Yes. Now LLVM_DEFAULT_TARGET_TRIPLE will be empty when the native target is not enabled.
check-llvm passed with this change (It fails when the native target is not enabled without this change. I only tested Linux build on x86 though.)

Sorry for the slow review.

Does this mean that LLVM_DEFAULT_TARGET_TRIPLE will now be empty when the native target isn't enabled (since LLVM_DEFAULT_TARGET_TRIPLE_default won't be set)? Does that cause any problems?

Thanks for reviewing.
Yes. Now LLVM_DEFAULT_TARGET_TRIPLE will be empty when the native target is not enabled.
check-llvm passed with this change (It fails when the native target is not enabled without this change. I only tested Linux build on x86 though.)

What about check-clang? What does clang do in this case if you try a compile without specifying an explicit triple?

Conceptually, requiring the user to explicitly pick a default target triple from one of the backends they have enabled seems cleaner than having it be empty to me. We'd error out if you don't have the native target enabled and didn't explicitly specify LLVM_DEFAULT_TARGET_TRIPLE. Would that work for you?

python3kgae added a comment.EditedOct 10 2022, 10:53 AM

Sorry for the slow review.

Does this mean that LLVM_DEFAULT_TARGET_TRIPLE will now be empty when the native target isn't enabled (since LLVM_DEFAULT_TARGET_TRIPLE_default won't be set)? Does that cause any problems?

Thanks for reviewing.
Yes. Now LLVM_DEFAULT_TARGET_TRIPLE will be empty when the native target is not enabled.
check-llvm passed with this change (It fails when the native target is not enabled without this change. I only tested Linux build on x86 though.)

What about check-clang? What does clang do in this case if you try a compile without specifying an explicit triple?

Conceptually, requiring the user to explicitly pick a default target triple from one of the backends they have enabled seems cleaner than having it be empty to me. We'd error out if you don't have the native target enabled and didn't explicitly specify LLVM_DEFAULT_TARGET_TRIPLE. Would that work for you?

check-clang fails as you expected :(
And set LLVM_DEFAULT_TARGET_TRIPLE to "" will also make check-clang fail.

My use case is native target disabled in llvm, which will fail check-llvm because LLVM_DEFAULT_TARGET_TRIPLE is set to native target which is not enabled.
That could be workaround by set LLVM_DEFAULT_TARGET_TRIPLE to "" or apply this patch.
And I also want to run check-clang, it will work by default, but it fails after set LLVM_DEFAULT_TARGET_TRIPLE "".

Is it possible to make both check-clang and check-llvm pass while native target is not enabled?
One solution could be add something like native_triple_enabled in lit config, then update all tests that check default_triple to check native_triple_enabled.
Is there another way to make it work?

Sorry for the slow review.

Does this mean that LLVM_DEFAULT_TARGET_TRIPLE will now be empty when the native target isn't enabled (since LLVM_DEFAULT_TARGET_TRIPLE_default won't be set)? Does that cause any problems?

Thanks for reviewing.
Yes. Now LLVM_DEFAULT_TARGET_TRIPLE will be empty when the native target is not enabled.
check-llvm passed with this change (It fails when the native target is not enabled without this change. I only tested Linux build on x86 though.)

What about check-clang? What does clang do in this case if you try a compile without specifying an explicit triple?

Conceptually, requiring the user to explicitly pick a default target triple from one of the backends they have enabled seems cleaner than having it be empty to me. We'd error out if you don't have the native target enabled and didn't explicitly specify LLVM_DEFAULT_TARGET_TRIPLE. Would that work for you?

check-clang fails as you expected :(
And set LLVM_DEFAULT_TARGET_TRIPLE to "" will also make check-clang fail.

My use case is native target disabled in llvm, which will fail check-llvm because LLVM_DEFAULT_TARGET_TRIPLE is set to native target which is not enabled.
That could be workaround by set LLVM_DEFAULT_TARGET_TRIPLE to "" or apply this patch.
And I also want to run check-clang, it will work by default, but it fails after set LLVM_DEFAULT_TARGET_TRIPLE "".

Is it possible to make both check-clang and check-llvm pass while native target is not enabled?
One solution could be add something like native_triple_enabled in lit config, then update all tests that check default_triple to check native_triple_enabled.
Is there another way to make it work?

How does check-clang behave if you set LLVM_DEFAULT_TARGET_TRIPLE to a triple for a backend that you do have enabled?

Sorry for the slow review.

Does this mean that LLVM_DEFAULT_TARGET_TRIPLE will now be empty when the native target isn't enabled (since LLVM_DEFAULT_TARGET_TRIPLE_default won't be set)? Does that cause any problems?

Thanks for reviewing.
Yes. Now LLVM_DEFAULT_TARGET_TRIPLE will be empty when the native target is not enabled.
check-llvm passed with this change (It fails when the native target is not enabled without this change. I only tested Linux build on x86 though.)

What about check-clang? What does clang do in this case if you try a compile without specifying an explicit triple?

Conceptually, requiring the user to explicitly pick a default target triple from one of the backends they have enabled seems cleaner than having it be empty to me. We'd error out if you don't have the native target enabled and didn't explicitly specify LLVM_DEFAULT_TARGET_TRIPLE. Would that work for you?

check-clang fails as you expected :(
And set LLVM_DEFAULT_TARGET_TRIPLE to "" will also make check-clang fail.

My use case is native target disabled in llvm, which will fail check-llvm because LLVM_DEFAULT_TARGET_TRIPLE is set to native target which is not enabled.
That could be workaround by set LLVM_DEFAULT_TARGET_TRIPLE to "" or apply this patch.
And I also want to run check-clang, it will work by default, but it fails after set LLVM_DEFAULT_TARGET_TRIPLE "".

Is it possible to make both check-clang and check-llvm pass while native target is not enabled?
One solution could be add something like native_triple_enabled in lit config, then update all tests that check default_triple to check native_triple_enabled.
Is there another way to make it work?

How does check-clang behave if you set LLVM_DEFAULT_TARGET_TRIPLE to a triple for a backend that you do have enabled?

Both check-clang and check-llvm will fail when set LLVM_DEFAULT_TARGET_TRIPLE to the triple with target enabled. :(

Hmm, I'm unable to repro that. On an x86-64 Linux machine, I set LLVM_TARGETS_TO_BUILD to just AArch64 and LLVM_DEFAULT_TARGET_TRIPLE to aarch64-linux-gnu. check-llvm and check-clang both worked for me. What values were you trying?

Hmm, I'm unable to repro that. On an x86-64 Linux machine, I set LLVM_TARGETS_TO_BUILD to just AArch64 and LLVM_DEFAULT_TARGET_TRIPLE to aarch64-linux-gnu. check-llvm and check-clang both worked for me. What values were you trying?

I'm building DirectX target which is still experimental.
But I tested amdgcn-amd-amdhsa too, it also fails.

Unfortunately I'm not set up for AMDGPU, so I can't test that locally. Just to confirm, check-llvm works for you if LLVM_DEFAULT_TARGET_TRIPLE is empty, but not if it's set to a triple corresponding to an enabled backend?

Unfortunately I'm not set up for AMDGPU, so I can't test that locally. Just to confirm, check-llvm works for you if LLVM_DEFAULT_TARGET_TRIPLE is empty, but not if it's set to a triple corresponding to an enabled backend?

Yes. check-llvm works if LLVM_DEFAULT_TARGET_TRIPLE is empty. ( except 2 OrcV2Examples in this patch).
Then set it to amdgcn-amd-amdhsa get lot of fails.

Unfortunately I'm not set up for AMDGPU, so I can't test that locally. Just to confirm, check-llvm works for you if LLVM_DEFAULT_TARGET_TRIPLE is empty, but not if it's set to a triple corresponding to an enabled backend?

Yes. check-llvm works if LLVM_DEFAULT_TARGET_TRIPLE is empty. ( except 2 OrcV2Examples in this patch).
Then set it to amdgcn-amd-amdhsa get lot of fails.

Hmm. Do you have an example of a failing test and why it's failing? (I see lots of amdhsa failures on my end, but I'm pretty sure it's just because I'm not set up correctly.) I'm trying to understand why the default triple is making a difference here, vs. just having the backend enabled or not enabled, and what the behavior is when the default triple is empty that's making the test pass.

Unfortunately I'm not set up for AMDGPU, so I can't test that locally. Just to confirm, check-llvm works for you if LLVM_DEFAULT_TARGET_TRIPLE is empty, but not if it's set to a triple corresponding to an enabled backend?

Yes. check-llvm works if LLVM_DEFAULT_TARGET_TRIPLE is empty. ( except 2 OrcV2Examples in this patch).
Then set it to amdgcn-amd-amdhsa get lot of fails.

Hmm. Do you have an example of a failing test and why it's failing? (I see lots of amdhsa failures on my end, but I'm pretty sure it's just because I'm not set up correctly.) I'm trying to understand why the default triple is making a difference here, vs. just having the backend enabled or not enabled, and what the behavior is when the default triple is empty that's making the test pass.

For check-llvm,
amdgcn-amd-amdhsa will cause many amdgpu test failures because amdhsa not work for some tests. Change it to amdgcn-amd- could avoid all amdgpu codeGen test fails.
The rest of fails might caused by amdgcn doesn't support all the things CPU supports and fail in instruction selection.
For default triple empty, these tests are ignored as unsupported by "# REQUIRES: default_triple".

For check-clang,
Lit will report fatal: Could not turn 'amdgcn-amd-' into Itanium ABI triple. Something for empty default triple.
amdgcn-amd-amdhsa will get many tests fail for cannot find ROCm device library.

FWIW this seems like a sensible change to me. The testing infrastructure for handling an empty triple is all there. It's sad that the clang tests don't pass, but that's a pre-existing issue with no default triple so I don't think it should block us here.

Conceptually, requiring the user to explicitly pick a default target triple from one of the backends they have enabled seems cleaner than having it be empty to me. We'd error out if you don't have the native target enabled and didn't explicitly specify LLVM_DEFAULT_TARGET_TRIPLE. Would that work for you?

I'm not necessarily opposed to some kind of warning here, but we would have to be careful not to error when the user explicitly sets an empty triple. This is fairly common when you want users of the compiler to have to choose a backend explicitly.

smeenai accepted this revision.Fri, Nov 4, 12:03 PM

Sorry, this fell off my radar. This seems like it solves your problem, it's clean enough, and the alternative I had in mind wasn't significantly better, so we can go ahead with this.

This revision is now accepted and ready to land.Fri, Nov 4, 12:03 PM

Sorry, this fell off my radar. This seems like it solves your problem, it's clean enough, and the alternative I had in mind wasn't significantly better, so we can go ahead with this.

Thanks for the review.
I'll see if it is possible to make check-clang work without native target next.

@python3kgae Just randomly spotted your patch. Thanks! (Why wouldn't Herald add me to the review..?)