This is an archive of the discontinued LLVM Phabricator instance.

Correct test for ARM targets
ClosedPublic

Authored by DazedOwl on May 22 2017, 7:11 AM.

Details

Summary

Test fails on ARM targets due to ABI name between define and void. Added reg ex to skip.

Diff Detail

Repository
rL LLVM

Event Timeline

DazedOwl created this revision.May 22 2017, 7:11 AM

Hi, apologies for not knowing the correct procedure for selecting reviewers (I'd be grateful if anyone wants to put me straight on this), I've just selected everyone who seems to have an interest.

It's a minute change so if anyone can give it the go ahead, please do.

Thanks
Glenn

rengolin edited edge metadata.Jun 6 2017, 2:46 AM

Where is this failing?

It's certainly passing on all the ARM bots...

http://llvm.validation.linaro.org/

Just FYI, when you submit a patch, use "git diff -U999" so that we can see the context in the web interface. Makes it easier to review. :)

tejohnson edited edge metadata.Jun 6 2017, 6:28 AM

I suppose this could also be fixed by adding a target triple so the ABI name is not added? Looks like most tests in that directory have one, which is presumably why they don't hit this. Unless we specifically want to test the handling of ARM bitcode here, can you do that instead?

I don't see why we should force the target, given that this is a CodeGen test, should worr (and be tested) on all targets.

I'm just curious as to where it fails, and how to make it fail on the bots, so that we can catch it again.

The fix is not wrong for the kind of problem described, I just can't see it happening.

--renato

Hi

Using a Linux Clang -cc1 version 3.4 based on LLVM 3.4 default target x86_64-pc-linux-gnu to build a simple test function:

clang -cc1 -emit-llvm -v -o loop.bc -triple arm7a-arm loop.cpp

we get:

define arm_aapcscc void @_Z4testv() #0 {

It's 'arm-aapcscc' that's giving us the problem.

Hi

Using a Linux Clang -cc1 version 3.4 based on LLVM 3.4 default target x86_64-pc-linux-gnu to build a simple test function:

That's a much older compiler than this test. Are you seeing this fail using a compiler of the same vintage as the test which was added a few months ago? I tried using the below triple on this test case (using current HEAD) and can't get it to add the ABI name.

clang -cc1 -emit-llvm -v -o loop.bc -triple arm7a-arm loop.cpp

we get:

define arm_aapcscc void @_Z4testv() #0 {

It's 'arm-aapcscc' that's giving us the problem.

DazedOwl added a comment.EditedJun 26 2017, 5:48 AM

Yes using current HEAD (rev 306279) we are still getting the same failure, for both Linux and Windows.

Yes using current HEAD (rev 306279) we are still getting the same failure, for both Linux and Windows.

Can you share your environment? It is surprising that this fails for you but it's currently passing on all buildbots: ARM, x86, Linux, Windows...

Can you share your environment? It is surprising that this fails for you but it's currently passing on all buildbots: ARM, x86, Linux, Windows...

Yes, what sort of details would actually be of use to you?

rengolin accepted this revision.Jul 20 2017, 4:12 AM

Apologies, you had already sent all I needed. :)

clang -cc1 -emit-llvm -v -o loop.bc -triple arm7a-arm loop.cpp

So, when I run this like the test, without -triple, I don't get the arm_aapcscc flag. On our systems, that defaults to armv7l-unknown-linux-gnueabihf. When not specifying gnueabi*, I do get the flag.

This looks like a bad behaviour, but given the complexity of ABIs, I'm not ready yet to call that foul. :)

This change is correct in the sense that different ARM targets will have different function flags by default, and since we're not using a specific target, we need to cover all bases, regardless of ABI issues that could be lurking underneath.

LGTM. Thanks, and sorry for the delay.

--renato

This revision is now accepted and ready to land.Jul 20 2017, 4:12 AM
dyung added a subscriber: dyung.Aug 25 2017, 2:37 AM

Upstream r311707 seems to have added two tests, CodeGen/profile-sample-accurate.c and Integration/thinlto_profile_sample_accurate.c which are failing for the same reason. Can this fix be applied to those changes as well?

This revision was automatically updated to reflect the committed changes.