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

[David Spickett via Phabricator via llvm-commits]

DavidSpickett marked 8 inline comments as done.
DavidSpickett added inline comments.


================
Comment at: include/llvm/Support/TargetParser.h:33
 // even if the back-end is not compiled with LLVM, plus we need to create a new
 // back-end to TableGen to create these clean tables.
 namespace X86 {
----------------
peter.smith wrote:
> 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.
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.


================
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);
 
----------------
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.


https://reviews.llvm.org/D53980