[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.

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.



================
Comment at: include/llvm/Support/AArch64TargetParser.h:1
+#ifndef LLVM_SUPPORT_AARCH64TARGETPARSERCOMMON_H
+#define LLVM_SUPPORT_AARCH64TARGETPARSERCOMMON_H
----------------
I think you'll need at least a license statement as a header here. See top of TargetParser.h for an example.


================
Comment at: include/llvm/Support/ARMTargetParser.h:1
+#ifndef LLVM_SUPPORT_ARMTARGETPARSER_H
+#define LLVM_SUPPORT_ARMTARGETPARSER_H
----------------
I think you'll need at least a license statement as a header here. See top of TargetParser.h for an example.


================
Comment at: include/llvm/Support/ARMTargetParser.h:231
+#endif
\ No newline at end of file

----------------
I guess clang-format will clean this up


================
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 {
----------------
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.


================
Comment at: lib/Support/AArch64TargetParser.cpp:1
+#include "llvm/Support/AArch64TargetParser.h"
+#include "llvm/ADT/StringRef.h"
----------------
Same missing license header comment here.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D53980