This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Get the AArch64 tests to work on Linux
ClosedPublic

Authored by rovka on Oct 12 2016, 1:10 PM.

Details

Summary

Mostly this just means changing the triple from aarch64-apple-ios to the generic aarch64--.
Only one test needs more significant changes, but GlobalISel already does the right thing so it's ok to just change the checks.

Diff Detail

Event Timeline

rovka updated this revision to Diff 74427.Oct 12 2016, 1:10 PM
rovka retitled this revision from to [GlobalISel] Get the AArch64 tests to work on Linux.
rovka updated this object.
rovka added reviewers: t.p.northover, qcolombet.
rovka added subscribers: llvm-commits, rengolin.
qcolombet added inline comments.Oct 12 2016, 2:16 PM
unittests/CodeGen/GlobalISel/CMakeLists.txt
3

That change shouldn't be part of this patch, should it?

t.p.northover edited edge metadata.Oct 12 2016, 2:16 PM

Certainly worth a try. Historically I've found generic-OS tests more trouble than they're worth, but I suppose we can sort that if they really do become a problem. Just one comment on the bigger change:

test/CodeGen/AArch64/GlobalISel/arm64-instructionselect.mir
1–3

It'd be better to specify -relocation-model=pic which should make sure the GOT gets used on Linux too. Hopefully that won't change anything else in this test.

t.p.northover added inline comments.Oct 12 2016, 2:33 PM
unittests/CodeGen/GlobalISel/CMakeLists.txt
3

I expect it's intentional. The linker on Linux has different name-resolution rules from macOS, I wouldn't be at all surprised if the unittest failed to link there without this line.

Certainly worth a try. Historically I've found generic-OS tests more trouble than they're worth, but I suppose we can sort that if they really do become a problem.

I just thought generic is better because it makes it easier to move to a new platform - you start by running the tests on it and fixing what breaks. Plus it doesn't add extra time to the tests, you just ensure coverage by adding more buildbots.
You probably have a better feel for this, so I don't mind changing to two run lines (one for iOS and one for Linux) if you think that's better.

test/CodeGen/AArch64/GlobalISel/arm64-instructionselect.mir
1–3

Ok, thanks, I'll give it a try. Stay tuned.

unittests/CodeGen/GlobalISel/CMakeLists.txt
3

Yup, it doesn't link without this. This might be a bug in the way our CMake files handle components, but I don't really know enough about our build system to tell.
@beanz : What do you think?

rovka updated this revision to Diff 74498.Oct 13 2016, 5:07 AM
rovka edited edge metadata.

Unfortunately using the PIC relocation model does something different for var_local. I think it makes sense to keep both PIC and default modes for Linux, as they are both different from iOS.

qcolombet accepted this revision.Oct 13 2016, 9:14 AM
qcolombet edited edge metadata.

LGTM.

unittests/CodeGen/GlobalISel/CMakeLists.txt
3

Thanks for the information!

This revision is now accepted and ready to land.Oct 13 2016, 9:14 AM
This revision was automatically updated to reflect the committed changes.