Page MenuHomePhabricator

[ARM, AArch64] Move ARM/AArch64 target parsers into separate files to enable future changes.
ClosedPublic

Authored by DavidSpickett on Nov 1 2018, 9:15 AM.

Details

Summary

This moves ARM and AArch64 target parsing into their
own files to enable future changes. They are still accessible
through TargetParser.h as before.

Several functions in AArch64 which were just forwarders to ARM
have been removed. All except AArch64::getFPUName were unused,
and that was only used in a test. Which itself was overlapping
one in ARM, so it has also been removed.

Diff Detail

Repository
rL LLVM

Event Timeline

DavidSpickett created this revision.Nov 1 2018, 9:15 AM
DavidSpickett changed the visibility from "Public (No Login Required)" to "No One".Nov 1 2018, 10:04 AM
DavidSpickett changed the visibility from "No One" to "Public (No Login Required)".
DavidSpickett edited the summary of this revision. (Show Details)Nov 2 2018, 3:25 AM
dmgreen added a subscriber: dmgreen.

Hello! Drive by comments. I've not looked in any depth.

include/llvm/Support/ARMTargetParser.h
189 ↗(On Diff #172145)

Some of these are now here twice.

lib/Support/ARMTargetParser.cpp
1 ↗(On Diff #172145)

You need to add file headers, same as in all other files in llvm.

425 ↗(On Diff #172145)

It's worth running clang-format over the patch, if you haven't done already.

I've no objections to splitting the headers apart, may be worth adding some x86, amdgpu folks on the review list with respect to my comment about splitting out their targets too.

include/llvm/Support/AArch64TargetParser.h
1 ↗(On Diff #172145)

I think you'll need at least a license statement as a header here. See top of TargetParser.h for an example.

include/llvm/Support/ARMTargetParser.h
1 ↗(On Diff #172145)

I think you'll need at least a license statement as a header here. See top of TargetParser.h for an example.

231 ↗(On Diff #172145)

I guess clang-format will clean this up

include/llvm/Support/TargetParser.h
33 ↗(On Diff #172145)

Looking at the contents of the file prior to the change we've got 4 namespaces making up Arm, AArch64, X86 and AMDGPU and afterwards we've split out Arm and AArch64 leaving just X86 and AMDGPU with Arm and AArch64 included separately.

Would it be worth breaking out X86 and AMDGPU into their own header files as well rather than leaving it half/half?

Either way I think the comment needs updating to at least point people at the header files.

lib/Support/AArch64TargetParser.cpp
1 ↗(On Diff #172145)

Same missing license header comment here.

unittests/Support/TargetParserTest.cpp
694 ↗(On Diff #172145)

Is this worth having here at all? Is there any meaning for it now that AArch64::getFPUName() has been removed?

This is assuming that we don't expect anyone to call AArch64::getFPUName(), if we do are they expected to know that they need to call ARM::getFPUName()?

If we do expect people to call the equivalent of AArch64::getFPUName() then we should provide an implementation.

Added headers and newlines at end of files.
Removed the duplicated declarations in the header.
Clang formatted and updated the comment in targetparser.h to point out that ARM/AArch64 namespaces have moved.

DavidSpickett marked 8 inline comments as done.Nov 5 2018, 3:53 AM
DavidSpickett added inline comments.
include/llvm/Support/TargetParser.h
33 ↗(On Diff #172145)

Updated the comment.

I don't know who that would be off the top of my head but I can ask on the mailing list. Anyone who does know feel free to add them.

unittests/Support/TargetParserTest.cpp
694 ↗(On Diff #172145)

The test itself is checking the result of AArch64::getDefaultFPU, which we also don't call otherwise. I can only assume llvm implies the FPU from the extensions regardless of what we put in the .def file.

I think this test is still worthwhile because it checks that this comment holds true:
ARMTargetParser.h:132:
The entries must appear in the order listed in ARM::FPUKind for correct
indexing
struct FPUName {

I agree that mixing ARM and AArch64 looks odd in context but it seems to be the current standard. Things like Endian were also forwarders to ARM but no one was using the AArch64 versions.

For this patch it made more sense to change this one instance than audit every other use of those functions. Maybe that is a good next step? There's a fixme that hints at the same thing:
// FIXME:This should be made into class design,to avoid dupplication.

DavidSpickett marked an inline comment as done.Nov 5 2018, 8:03 AM

Apologies for the delay in responding, I've taken a look in a bit more detail at how AArch64 specifies the FPU.

unittests/Support/TargetParserTest.cpp
694 ↗(On Diff #172145)

I think the instance of the same code in testARMCPU will check that the order of the definitions in ARMTargetParser.h are correct.

As far as I can tell the only thing that this is currently testing is that the FPU names used in the test cases that call testAArch64CPU happen to match in the ARM::getFPUName(), but I don't think that adds much value and at worst case could be confusing.

Personally I think that if we are going to remove the unused (except by the test) forwarding functions from the AArch64 namespace we should remove the test. Alternatively I think we should at least write a comment justifying why it is there. I don't want to get too hung up about this though I'm sure there will be further changes that will sort this out.

As to what happens with AArch64:
I think all AArch64 cpu targets in AArch64.td including generic have FeatureFPARMv8 (there is no -mfpu option) so there is no current need to call the function. In fact the only way I've been able to find a way of not using the fpu is via -mgeneral-regs-only.

Interesting you mention about clang not calling AArch64::getDefaultFPU(). ARM::getDefaultFPU() is called from ARMTargetInfo::initFeatureMap().

There was a bug reported recently (https://bugs.llvm.org/show_bug.cgi?id=33480) where __ARM_FEATURE_CRC32 was not being defined for AArch64 when -mcpu=generic was used with -march=armv8.1-a , but it was for ARM and the difference was that ARMTargetInfo had an implementation of initFeatureMap() but AArch64TargetInfo used the default TargetInfo::initFeatureMap().

Perhaps if we were to implement initFeatureMap() we'd need AArch64::getDefaultFPU()?

DavidSpickett added inline comments.Nov 6 2018, 1:21 AM
unittests/Support/TargetParserTest.cpp
694 ↗(On Diff #172145)

Ah yes you're right ARM already covers the ordering. I'll remove the test for now, we can bring it back (probably in a cleaner way) if we do need getDefaultFPU for initFeatureMap.

Bryan Chan on the mailing list also reported this with dotprod:
"It seems that currently mandatory features in the various ARMv8.x architectures are not enabled in cfe by default, which is surprising and inconsistent with GCC's behavior."

It's one of the things we want to address in general with this work.

DavidSpickett retitled this revision from [ARM, AArch64] Move ARM/AAch64 target parsers into separate files to enable future changes. to [ARM, AArch64] Move ARM/AArch64 target parsers into separate files to enable future changes..
DavidSpickett edited the summary of this revision. (Show Details)

Removed the default FPU test from the AArch64 target parser tests. Updated the commit message to reflect this.

DavidSpickett marked 2 inline comments as done.Nov 8 2018, 1:38 AM
peter.smith accepted this revision.Nov 12 2018, 4:11 AM

Thanks for the update. This looks good to me. May be worth waiting till tomorrow before committing to give the US timezone a last chance to object.

This revision is now accepted and ready to land.Nov 12 2018, 4:11 AM

Small change to move all the declarations that use the .def files into their respective headers.

No problem, I'm still ok with the changes.

This revision was automatically updated to reflect the committed changes.