Page MenuHomePhabricator

[ARM][AArch64] Cortex-A75 and Cortex-A55 tests
ClosedPublic

Authored by samparker on Aug 15 2017, 3:35 AM.

Details

Summary

Add frontend tests for Cortex-A75 and Cortex-A55, Arm's latest big.LITTLE A-class cores. They implement the ARMv8.2-A architecture, including the cryptography and RAS extensions, plus the optional dot product extension. They also implement the RCpc AArch64 extension from ARMv8.3-A.

Cortex-A75:
https://developer.arm.com/products/processors/cortex-a/cortex-a75

Cortex-A55:
https://developer.arm.com/products/processors/cortex-a/cortex-a55

Diff Detail

Repository
rL LLVM

Event Timeline

samparker created this revision.Aug 15 2017, 3:35 AM
rengolin edited edge metadata.Aug 18 2017, 4:09 AM

This change seems to have nothing to do with adding core support but exposing features from CPU names.

If this was required to "support" the new cores in Clang, why wasn't it needed before?

If this is a new, more generic way, of getting support, shouldn't you remove the specialist code that already does that for all other cores?

cheers,
--renato

lib/Driver/ToolChains/Arch/ARM.cpp
90 ↗(On Diff #111140)

Why are you returning bool if you're not using the result?

348 ↗(On Diff #111140)

Isn't this conditional redundant with what the function does?

SjoerdMeijer edited edge metadata.Aug 18 2017, 4:27 AM

"They also implement the RCpc AArch64 extension from ARMv8.3-A."

Perhaps you need to explain why a v8.2 core implements a v8.3 extension?

lib/Driver/ToolChains/Arch/ARM.cpp
92 ↗(On Diff #111140)

Nit: just for readability I would prefer an early exit:

if (CPU == "generic")

return false;
102 ↗(On Diff #111140)

Nit: you're not checking the return value (so you could simplify this function, but I don't have a strong opinion on this).

348 ↗(On Diff #111140)

CPUName != "generic" is also checked in function DecodeARMFeaturesFromCPU.

test/Driver/arm-cortex-cpus.c
261 ↗(On Diff #111140)

Just checking: why has the default cpu changed from generic to cortex-a55, and secondly, is that what we want?

Ah, my message crossed with Renato's; I only noticed it after submitting mine (looks like we agree though:-))

samparker retitled this revision from [ARM][AArch64] Cortex-A75 and Cortex-A55 support to [ARM][AArch64] Cortex-A75 and Cortex-A55 tests.Aug 18 2017, 5:03 AM
samparker edited the summary of this revision. (Show Details)

Thanks guys, I will sort my logic out.

Renato, the new Decode function is really just there to allow me to double check the target features. This is already possible with AArch64 and it just felt right to also be able to test ARM in the same way. I've renamed the title so others aren't mislead.

cheers,
sam

test/Driver/arm-cortex-cpus.c
261 ↗(On Diff #111140)

I was thinking about this on the way to work, and no I don't think it is what we want! I will change this.

samparker updated this revision to Diff 111664.Aug 18 2017, 6:24 AM

Reverted the default cpu v8.2-a to generic, I will update D36667 accordingly. Also fixed up the boolean issues.

I'm happy with the patch, but I'll let @SjoerdMeijer approve.

SjoerdMeijer accepted this revision.Aug 18 2017, 7:09 AM

Looks good to me too.

Two nits (no new review required): one is inlined, and the other one in the summary: ARMv8.2-A => Armv8.2-A :-/

test/Driver/arm-dotprod.c
2 ↗(On Diff #111664)

Perhaps also test "v8.0" and add -march=armv8a?

This revision is now accepted and ready to land.Aug 18 2017, 7:09 AM
This revision was automatically updated to reflect the committed changes.