This is an archive of the discontinued LLVM Phabricator instance.

Add ARM big endian Target (armeb, thumbeb)
ClosedPublic

Authored by cpirker on Mar 17 2014, 7:23 AM.

Details

Reviewers
cpirker
Summary

Hi all,

This patch adds a big endian ARM target description.
(A corresponding target machine has been committed to llvm-commits D3095.)
Please review.

Thanks,
Christian

Diff Detail

Event Timeline

Hi Christian,

Again, just a round-the-edges review. Just a few comments:

  1. Could you add the ACLE predefine __ARM_BIG_ENDIAN? (set to 1 for big endian, undefined for little endian, see section 6.3 in http://infocenter.arm.com/help/topic/com.arm.doc.ihi0053b/IHI0053B_arm_c_language_extensions_2013.pdf)
  2. Would like to see tests for the predefine(s) -- ARMEL, ARMEB, __ARM_BIG_ENDIAN

Again, how is the testing on the AArch64 side? I think it was specifically your ARM_BE test case that I noticed didn't seem to be there on AArch64.

Regards,

Bernie

lib/Basic/Targets.cpp
4327

Can I say -target armeb-none-darwin-...? What happens if I do?

lib/Driver/Tools.cpp
475

I wonder if it is a bug that we don't return false for thumb (not asking you to fix it)

1774–1775

Again, seems odd that there is no equivalent for thumb (and, again, not asking you to fix)

cpirker updated this revision to Unknown Object (????).Mar 19 2014, 3:54 AM

Hi Bernie,

I added the ACLE predefine ARM_BIG_ENDIAN
It will be tested also in "test/Preprocessor/init.c", like the predefine(s) --
ARMEL, and ARMEB__.

Regards,
Christian

Ah - ARMEL and ARMEB tests were already there. Sorry, missed them the first time.

I'd like some 'NOT' checks. It may be best to do these as second tests in init.c, so that you don't confine the check to the range between two matches. I'd like to see that ARM_BIG_ENDIAN and ARMEB__ are not defined in the 'arm' case, and that ARMEL is not defined in the armeb case.

My question about -target armeb-none-darwin still stands (re inline comment from before).

Other than those points, LGTM, with the disclaimer that I'm no expert on the various platform triples you're setting up.

More generally, I think I'd like to see a CLI option to select endianness - I would prefer that as the 'user interface' over relying on another special triple name. That doesn't have to be part of this patch, though. (I haven't - yet - looked to see what other endian-bending targets do.)

cpirker updated this revision to Unknown Object (????).Mar 27 2014, 11:32 AM

Hi Tim,

I added negative tests for predefined macros in "test/Preprocessor/init.c".

Thanks,
Christian

I committed this patch as r205008.

lib/Basic/Targets.cpp
4327

This is not supported and causes an unknown target triple message.
The behaviour is the same as for "-target aarch64-none-darwin".

cpirker accepted this revision.May 15 2014, 6:30 AM
cpirker added a reviewer: cpirker.
This revision is now accepted and ready to land.May 15 2014, 6:30 AM
cpirker closed this revision.May 15 2014, 6:31 AM