This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Adjust aarch64 constrained intrinsics tests and un-XFAIL
ClosedPublic

Authored by john.brawn on Jan 26 2022, 8:58 AM.

Details

Summary

Remove the checking of the generated asm, as that's already tested elsewhere, and adjust some tests that were expecting the wrong intrinsic to be generated.

Diff Detail

Event Timeline

john.brawn created this revision.Jan 26 2022, 8:58 AM
john.brawn requested review of this revision.Jan 26 2022, 8:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2022, 8:58 AM
fhahn added a subscriber: fhahn.Jan 27 2022, 2:22 AM

Does this clang test actually need to check the generated assembly? Shouldn't it be enough to check that the correct intrinsics are generated?

Matt added a subscriber: Matt.Jan 27 2022, 5:44 AM

Does this clang test actually need to check the generated assembly? Shouldn't it be enough to check that the correct intrinsics are generated?

I could remove the CHECK-ASM checks, but the other xyz-constrained.c tests (including non-aarch64 ones) all do this and it feels a bit weird to just do that to this test.

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 6:04 AM
fhahn added a comment.Mar 17 2022, 5:15 AM

Does this clang test actually need to check the generated assembly? Shouldn't it be enough to check that the correct intrinsics are generated?

I could remove the CHECK-ASM checks, but the other xyz-constrained.c tests (including non-aarch64 ones) all do this and it feels a bit weird to just do that to this test.

I guess the question is if there is a convincing reason to check the assembly in this test? Unless it is required for the test, checking for assembly makes the test more fragile than they need to be. Also, is there any difference in the generated IR between X86 and AArch64? If not, the tests should probably be unified.

llvm/test/CodeGen/AArch64/fp-intrinsics-vector.ll is testing the asm generation, so it probably is fine to remove that here so I will. This test is specifically testing arm_neon.h intrinsics though, so wouldn't make sense unifying with other architectures.

kpn added a comment.Mar 17 2022, 6:54 AM

It's been a while, but I think the aarch64-neon-intrinsics-constrained.c test is trimmed down from the aarch64-neon-intrinsics.c test. Shouldn't the constrained and non-constrained end-to-end tests be treated the same?

fhahn added a comment.Mar 17 2022, 7:06 AM

It's been a while, but I think the aarch64-neon-intrinsics-constrained.c test is trimmed down from the aarch64-neon-intrinsics.c test. Shouldn't the constrained and non-constrained end-to-end tests be treated the same?

Are you referring to https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGen/aarch64-neon-intrinsics.c? This one doesn't check assembly.

kpn added a comment.Mar 17 2022, 7:16 AM

It's been a while, but I think the aarch64-neon-intrinsics-constrained.c test is trimmed down from the aarch64-neon-intrinsics.c test. Shouldn't the constrained and non-constrained end-to-end tests be treated the same?

Are you referring to https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGen/aarch64-neon-intrinsics.c? This one doesn't check assembly.

Ah, well, I guess that wasn't it, then. It's been a while, and I've been all over the place.

A clang-only test that just tests for the correct IR being emitted won't trigger the failures in the backend. An LLVM-only test that starts from that IR can show those failures, but it also runs the risk over time of getting out of sync with the IR emitted by clang. An end-to-end test will always be in sync, and it will show those failures that I ran into plus anything else that crops up over time. If there's another way, or if there is another approach, then it might be possible there's a way to get all the testing done without running into those issues. I just don't know what it is.

john.brawn retitled this revision from [AArch64] Adjust aarch64-neon-intrinsics-constrained test and un-XFAIL to [AArch64] Adjust aarch64 constrained intrinsics tests and un-XFAIL.
john.brawn edited the summary of this revision. (Show Details)

Also adjust aarch64-v8.2a-fp16-intrinsics-constrained.c in this patch (was previously done in
D115620) and remove the checks of the generated asm.

kpn accepted this revision.Apr 12 2022, 10:36 AM

I trust you on the instruction set changes. LGTM.

This revision is now accepted and ready to land.Apr 12 2022, 10:36 AM
This revision was landed with ongoing or failed builds.Apr 14 2022, 8:52 AM
This revision was automatically updated to reflect the committed changes.