This is an archive of the discontinued LLVM Phabricator instance.

Fix tests defaulting to incorrect triples on AIX
ClosedPublic

Authored by Jake-Egan on Sep 21 2021, 10:23 AM.

Details

Summary

The tests only specify -march, so when the tests are run on AIX the target OS defaults to AIX, which causes the tests to misbehave.

This patch constrains the tests by specifying -mtriple instead of -march.

Diff Detail

Event Timeline

Jake-Egan created this revision.Sep 21 2021, 10:23 AM
Jake-Egan requested review of this revision.Sep 21 2021, 10:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2021, 10:23 AM
Jake-Egan edited the summary of this revision. (Show Details)Sep 21 2021, 10:34 AM
Jake-Egan edited the summary of this revision. (Show Details)

Just a general question, how did you determine the OS and environment type used in the triple?

Just a general question, how did you determine the OS and environment type used in the triple?

From other tests that set the triple.

Should they be -mtriple=foo-unknown-linux or just -mtriple=foo, i.e. foo-unknown-elf? Given they've worked fine on Linux, FreeBSD and macOS for years I assume they don't care about the OS part of the triple (except for whatever AIX is doing that causes them to break...) and so should just use a generic ELF triple?

jsji requested changes to this revision.Sep 21 2021, 1:39 PM
jsji added a subscriber: jsji.

I don't think this is a correct way to fix all these for AIX.

Since we don't really support those arch on AIX, we should update getMultiarchTriple to resolve the arch to correct triple, instead of trying to update all existing tests.

This revision now requires changes to proceed.Sep 21 2021, 1:39 PM
MaskRay requested changes to this revision.EditedSep 21 2021, 1:50 PM
MaskRay added a subscriber: MaskRay.

The tests only specify -march, so when the tests are run on AIX the target OS defaults to AIX, which causes the tests to misbehave.

Can you confirm? I don't think so.

See llvm/lib/Support/Triple.cpp:getDefaultFormat which is used to normalize a triple when the format is not explicitly specified. ELF is the default binary format for everything except wasm.
This does give ELF a privilege but it is just how things work:)

The tests only specify -march, so when the tests are run on AIX the target OS defaults to AIX, which causes the tests to misbehave.

Can you confirm? I don't think so.

See llvm/lib/Support/Triple.cpp:getDefaultFormat. ELF is the default binary format for everything except wasm.
This does give ELF a privilege but that is just how things work:)

-march is defined to be "the native triple with the CPU replaced with the argument" and has caused problems in the past. It's not the same as -mtriple where there are defaults that don't depend on the native triple.

daltenty added a comment.EditedSep 21 2021, 2:11 PM

Hmm, for context the error is related to the relocation model:

 ./bin/llc -filetype=obj -march=sparcv9 --relocation-model=static < foo.ll
llc: error: '<stdin>': invalid relocation model, AIX only supports PIC

I guess if we wanted to side step this issue maybe we could tie that error to the object file format (i.e. XCOFF) instead and presumably we wouldn't see this error since the default is ELF?

MaskRay added a comment.EditedSep 21 2021, 2:56 PM

The tests only specify -march, so when the tests are run on AIX the target OS defaults to AIX, which causes the tests to misbehave.

Can you confirm? I don't think so.

See llvm/lib/Support/Triple.cpp:getDefaultFormat. ELF is the default binary format for everything except wasm.
This does give ELF a privilege but that is just how things work:)

-march is defined to be "the native triple with the CPU replaced with the argument" and has caused problems in the past. It's not the same as -mtriple where there are defaults that don't depend on the native triple.

Looks like -march may be a bit misleading due to llvm/lib/Support/Triple.cpp:getDefaultFormat returning ELF for many machines.

For example, on arm64 macOS, TheTarget->createTargetMachine takes a sparcv9-apple-darwin20.3.0 triple while the binary format is ELF since Darwin doesn't support sparcv9.

I think switching to -mtriple= makes sense to me. Either -mtriple=sparc9 or -mtriple=sparc9-unknown-elf works for me, perhaps the former for brevity.

The tests only specify -march, so when the tests are run on AIX the target OS defaults to AIX, which causes the tests to misbehave.

Can you confirm? I don't think so.

See llvm/lib/Support/Triple.cpp:getDefaultFormat. ELF is the default binary format for everything except wasm.
This does give ELF a privilege but that is just how things work:)

-march is defined to be "the native triple with the CPU replaced with the argument" and has caused problems in the past. It's not the same as -mtriple where there are defaults that don't depend on the native triple.

Looks like -march may be a bit misleading due to llvm/lib/Support/Triple.cpp:getDefaultFormat returning ELF for many machines.

For example, on arm64 macOS, TheTarget->createTargetMachine takes a sparcv9-apple-darwin20.3.0 triple while the binary format is ELF since Darwin doesn't support sparcv9.

I think switching to -mtriple= makes sense to me. Either -mtriple=sparc9 or -mtriple=sparc9-unknown-elf works for me, perhaps the former for brevity.

So, the _triple_ is always exactly what I said. But if that's a nonsensical triple, such as sparcv9-apple-darwin20.3.0, then yes things can get weird and it may end up falling back on ELF until suddenly that case gets implemented properly.

Jake-Egan updated this revision to Diff 374332.EditedSep 22 2021, 12:45 PM

Updated to use -mtriple=foo.

MaskRay accepted this revision.Sep 22 2021, 12:50 PM

Thanks!

daltenty accepted this revision.Sep 24 2021, 8:20 AM
jsji accepted this revision.Sep 27 2021, 6:52 AM

I am OK with this if others accept it. But as I mentioned, this is not a good way to fix it -- we will have to deal with any new testcases using -march in the future.

This revision is now accepted and ready to land.Sep 27 2021, 6:52 AM

I am OK with this if others accept it. But as I mentioned, this is not a good way to fix it -- we will have to deal with any new testcases using -march in the future.

No, tests that use -march= are broken, they will by definition have host-dependent behaviour.

jsji added a comment.Sep 27 2021, 7:07 AM

they will by definition have host-dependent behaviour.

Yes, that is why I am asking to update the host (*AIX*) default behaviors for those arch we don't support -- we don't have to default to XCOFF for those arch on AIX.

they will by definition have host-dependent behaviour.

Yes, that is why I am asking to update the host (*AIX*) default behaviors for those arch we don't support -- we don't have to default to XCOFF for those arch on AIX.

My point is this is the correct way to fix these broken tests; you were saying this was not a good way to fix it.

jsji added a comment.Sep 27 2021, 7:13 AM

they will by definition have host-dependent behaviour.

Yes, that is why I am asking to update the host (*AIX*) default behaviors for those arch we don't support -- we don't have to default to XCOFF for those arch on AIX.

My point is this is the correct way to fix these broken tests; you were saying this was not a good way to fix it.

:), you are right. I agree that this is the correct way.
I just want to make sure we also update our host default behaviors so that we don't have to pay the additional maintenance efforts for future testcases for other targets.

This revision was landed with ongoing or failed builds.Sep 27 2021, 8:31 AM
This revision was automatically updated to reflect the committed changes.

Due to its weird semantics, -march is probably only suitable for quick local testing but not for the testsuite.