Page MenuHomePhabricator

More processors support under AARch64 state for auto detection.
Needs RevisionPublic

Authored by supra on Sep 16 2016, 5:42 AM.

Details

Summary

Support for ARMv8-A based APM X-Gene-1 Processor

NEW DIFFERENTIAL REVISION:

  1. Adds support for Applied Micro (APM) X-Gene-1 processor.
    • Auto detection (-mcpu=native) under AArch64 state
    • Correct set of pre-defined macros for the features of this model
    • Currently uses "generic" scheduler. Will be enhanced soon.
  2. Adds support for ARM Cortex-A53, A57, A72 and A73
    • Auto detection (-mcpu=natie) under AArch64 state

Diff Detail

Event Timeline

supra updated this revision to Diff 71621.Sep 16 2016, 5:42 AM
supra retitled this revision from to More processors support under AARch64 state for auto detection..
supra updated this object.
supra updated this revision to Diff 71628.Sep 16 2016, 6:22 AM

Updating D24661: More processors support under AARch64 state for auto detection.
Adding more files to the changeset.

rengolin edited edge metadata.Sep 18 2016, 11:20 AM

Those are just the tests, where is the detection code?

supra updated this revision to Diff 71773.EditedSep 19 2016, 12:27 AM
supra edited edge metadata.

Adding all the files in the changeset.

(Sorry, not yet well versed with arc)

t.p.northover added inline comments.Sep 19 2016, 1:39 AM
lib/Support/Host.cpp
1122–1128 ↗(On Diff #71773)

I don't like how this scales with the many vendors. We'll end up with this loop duplicated half a dozen times before we're done.

Perhaps convert the strings to numbers and use std::lower_bound on a static array?

rengolin added inline comments.Sep 19 2016, 1:43 AM
lib/Support/Host.cpp
1122–1128 ↗(On Diff #71773)

Could we maybe add this to the target parser?

jgreenhalgh added inline comments.
lib/Target/AArch64/AArch64.td
259 ↗(On Diff #71773)

This disagrees with the GCC implementation of -mcpu=xgene1 and the xgene-1 processors available in the GCC compile farm (gcc113.osuosl.org is an APM X-Gene Mustang board). Are you sure that the CRC feature should be enabled?

This comment also applies to the lib/Target/ARM/ARM.td portion of your patch.

supra updated this revision to Diff 72146.Sep 22 2016, 3:00 AM

CRC is not available on APM X-Gene-1. Removed CRC for
-mcpu=xgene1.

As adding XGene1 goes, the change is reasonable, but there's an auto-detection left-over.

A couple of LLVM-specifics...

  • Clang and LLVM are in different repositories, so we'll need two different reviews, one for each part.
  • The Clang tests for xgene are good, but we'll also need TargetParser tests (see ./unittests/Support/TargetParserTest.cpp)

A few other comments inline.

Thanks!
--renato

include/llvm/Support/ARMTargetParser.def
237 ↗(On Diff #72146)

@jgreenhalgh, is this the same defaults as GCC?

include/llvm/Support/TargetParser.h
165 ↗(On Diff #72146)

this change now is unnecessary

lib/Support/Host.cpp
1098 ↗(On Diff #72146)

As discussed over the mailing list, please discard this change.

jgreenhalgh added inline comments.Sep 22 2016, 3:24 AM
include/llvm/Support/ARMTargetParser.def
237 ↗(On Diff #72146)

Yes, the XGene-1 change looks good to me now. Thanks for the update Supra.

supra updated this revision to Diff 72235.Sep 22 2016, 10:20 PM

Removed auto detection for -mcpu=native

Two small style comments, but it looks good to me with those changes.

Thanks!
--renato

include/llvm/Support/TargetParser.h
165 ↗(On Diff #72235)

There's no need to keep the comma now that you're not changing the list. Once you do, you can leave another on the new tag.

unittests/Support/TargetParserTest.cpp
549 ↗(On Diff #72235)

keep this in the same line, pls.

supra updated this revision to Diff 72419.Sep 25 2016, 1:25 AM

Fixed some coding style issues.

rengolin accepted this revision.Sep 27 2016, 10:03 AM
rengolin edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Sep 27 2016, 10:03 AM
rengolin requested changes to this revision.Sep 27 2016, 10:54 AM
rengolin edited edge metadata.

Oh, I just noticed again, sorry (swapping a bit). This code doesn't even compile.

You have three patches here:

  1. AArch64 changes, adding XGene1: You need to add the model and descriptions on all related table-gen files. You're defining it but not using it.
  2. ARM changes, adding XGene1: You need to define in the same way you did for AArch64, because the usage can't find the declaration, which is in the AArch64.td.
  3. Clang changes: the tests rely on the targets being there, so need to be committed *after* the LLVM patch.

For 1 and 2, you'll need two separate patches, with similar changes and appropriate unit tests.

For 3, you'll need both ARM and AArch64 tests (-target armv8a and -target aarch64), and make sure to mention the patch *depends* on the LLVM ones.

Sorry for the confusion, and please, before submitting the three patches, make sure that they build and pass "check-all" individually, as well as altogether.

cheers,
--renato

This revision now requires changes to proceed.Sep 27 2016, 10:54 AM