[PATCH] D20089: Adding a TargetParser for AArch64

[Renato Golin via cfe-commits]

rengolin added a comment.

Hi Jojo,

I've just mapped these changes to the current ARM implementation and they look correct. I only have two minor comments and I'm happy with it, but I'll let Bradley have the final review.

Bradley,

If history is of any help, Jojo will have to change a few things once she makes Clang and others use it. It shouldn't be a huge deal, but I believe there will be some teething issues. Once that's done, our next step would be to make into a class.

From what I see, there are two steps to the merge:

1. Create an interface, from which both ARM and AArch64 derive, and remove all obviously redundant methods (direct calls to ARM). An alternative would be to have AArch64 extend ARM, but I think that would be unfair to MIPS, as they could profit from this, too. Since we have no proof of that, I'd be ok with the second option, if people think it isn't worth being generic.

2. See how much of the tables can be the same, but having different contents, in a way that all the other non obvious redundancies can be resolved (all parse methods with tables). From the looks of it, almost everything can be merged, and most methods can be reduced to the base class.

IMO, each should be done separately, as they're both big changes. The first change will also come with a huge list of changed files in both Clang and LLVM, so we need to keep it as simple as possible.

cheers,
--renato


================
Comment at: include/llvm/Support/TargetParser.h:173
@@ +172,3 @@
+StringRef getArchName(unsigned ArchKind);
+bool getArchFeatures(unsigned ArchKind, std::vector<const char *> &Features);
+unsigned getArchAttr(unsigned ArchKind);
----------------
Nitpick, move this declaration with the others that use &Features.

================
Comment at: include/llvm/Support/TargetParser.h:184
@@ +183,3 @@
+unsigned  getDefaultExtensions(StringRef CPU, unsigned ArchKind);
+StringRef getDefaultCPU(StringRef Arch, std::vector<const char *> &Features);
+
----------------
You don't need the &Features any more. Same of the implementation in the cpp file.


Repository:
  rL LLVM

http://reviews.llvm.org/D20089