AArch64: High level review

[Jim Grosbach]

Hi Tim,

Thanks again for all your work on this backend. Very impressive. Rather than an overly onerous line-by-line commentary, here's some specific areas I'm curious about, and a few questions and suggestions.

Simple nitty stuff first to get it out of the way. There are a few formatting cleanups to be done. Hard tab characters, 80 column violations, etc.. Nothing massive, but it would be worth a pass through the code to fix the few issues up. The only consistent bit in this regard is that the .td files don't currently follow the normal conventions for braces (they put the open brace on a new line, when it should be trailing on the opening line just like in C++ code). Some files are missing a block comment at the top (e.g., AArch64InstrInfo.h).

Pretty much all of the below should be taken as suggestions, not requirements for getting this moved out of "experimental". The only exception being the MC/Parser cycle bit, which you're already dealing with.

In no particular order---

Register file organization:
Nice use of "foreach" to define the numbed registers. Keeps things nice and concise.

LR and FP should be explicitly called out as being "special" in their register names, in particular for printing. It's much easier to read assembly code like "ldr x2, [fp, #16]" and know that's a stack reference than remember which numbered register is the frame pointer. Yes, that will muck about with the ordering of the register enum; however, relying on that is fraught with danger anyway and I advise against it if you have an alternative.

VFP registers and SIMD registers shouldn't need to have separate register definitions. This kind of register name aliasing is pretty atrocious, but even so, some of the pain can be reduced a bit. Mainly, the subreg relationship that's actually just a naming alias pretty awkward. A better option is to use the RegisterOperand class with a dedicated print method. That way you can have only a single actual RegisterClass but still have the instruction printer use the right names for the registers contextually depending on the instruction being printed. Something like,
def V128 : RegisterOperand<FPR128, "printVectorRegisterOperand">;
That print method can reference the alternate name (tablegen will do this for you, too, via RegAltNameIndices) for easy printing.

Codegen:

I would strongly consider getting rid of the constant island pass. This arch has good enough constant materialization instructions that it's not necessary for most values. I'd get rid of it for ARM32 as well if we were dumping pre-v7 support, honestly. For the rare cases where it is better (atypical 64 bit constants and 128 bit vector constants, e.g.), putting the constants in a separate section and accessing as a normal global value is almost certainly going to be better. Have a look at how x86_64 does this (lit8 and lit16 sections and related) for an idea of what I'm thinking here. Most of the constants normal will use are going to be materializable in one or two instructions. Even if not, putting islands in the text section is likely going to play absolute havoc with the branch predictor and speculative execution hardware. Better to put data outside of the text section.

Assembler:

The target description <--> parser cycle has already been talked about elsewhere, so I won't belabor it. Generally speaking, the parser should be considered at a higher level than the target description (just like isel), and so the MCTargetDesc stuff shouldn't be including anything from it (or anything else target specific outside of the MCTargetDesc directory, for the most part).

validateInstruction() has a comment about BFI that talks about the disassembler. I'm confused. What does the disassembler have to do with the parser? What is this code really trying to do?

There are a lot of PostEncoderMethod hooks. Those make me nervous, as they're typically for things like the shared .td file definitions for the ARM mode and Thumb2 mode VFP and NEON instructions in the ARM backend. I would not have expected to see any in an architecture like this. If nothing else, it's an additional callback out to hand-written C++ code, so there can be assembler performance issues.

Diagnostic information for out of range immediates can be significantly improved. Consider:
x.s:2:14: error: invalid operand for instruction
ldr x1, [x2, #1111]
             ^
This would be much better if the diagnostic referenced why it's invalid and mentioned the legal range and alignment restrictions, if any. You can do this via the DiagnosticType field of the AsmOperand class in TableGen. The ARM backend uses this a bit, though not as much as I'd like. Enough that there should be a reasonable example, anyway. In general, these allow for much better diagnostics for anything that's failing during the instruction match process rather than during a custom parse method.


Testing:
Very nice test cases. In particular I like that you have lots of testing of edge cases for instruction operand ranges and negative testing for expected diagnostics from the assembler.


Thanks again for contributing this backend. Great to see it.

-Jim
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130204/a6693f44/attachment.html>