[PATCH] D53980: [ARM, AArch64] Move ARM/AAch64 target parsers into separate files to enable future changes.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

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



================
Comment at: unittests/Support/TargetParserTest.cpp:694
   unsigned FPUKind = AArch64::getDefaultFPU(CPUName, AK);
-  pass &= AArch64::getFPUName(FPUKind).equals(ExpectedFPU);
+  pass &= ARM::getFPUName(FPUKind).equals(ExpectedFPU);
 
----------------
DavidSpickett wrote:
> peter.smith wrote:
> > 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.
> 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.
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()?



https://reviews.llvm.org/D53980