Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I think I have more comments than are strictly justified by how I feel about this patch. Generally, it *is* good, I just think it could be streamlined to be even better.
Tim.
lib/Target/AArch64/AArch64InstrFormats.td | ||
---|---|---|
8633–8636 ↗ | (On Diff #22386) | I don't think this is the best location for these changes. TokanAliases are very different from the bulk of the file, and are at the end of the file because of that. |
8668–8669 ↗ | (On Diff #22386) | These names are non-obvious. The AArch64 assembly syntax actually uses "L" for release. I'd suggest actually using "Acq" and "Rel" to make it more obvious for readers. We're not constrained for space here. |
8685–8690 ↗ | (On Diff #22386) | You should probably put BaseCAS and BaseCASP together. For a long while I thought this whole indirection was pointless (until I saw CASP). |
8693 ↗ | (On Diff #22386) | "BaseLD" is far too generic as a name for something that's only applicable to these atomicrmw operations. |
8715–8716 ↗ | (On Diff #22386) | I think the factoring of the multiclass hierarchy is inconsistent with the rest of the backend here. The general convention is that the capitalised prefix of the instruction mirrors the mnemonic, with suffixes (capitalised or not) showing the variant. Obviously, that's not always possible, but here I think it is. In this case, I'd expect in AArch64InstrInfo.td something like defm LDADD : ... defm LDADDA : ... defm LDADDL : ... defm LDADDAL : ... ... with the second-level hierarchy appending a B/H/W/X as needed. See the usual loads & stores for example (though they're confounded slightly by both "ldrb w0, [x0]" and "ldr b0, [x0]" being byte loads). |
8731–8732 ↗ | (On Diff #22386) | It's also rather odd that the "BaseAliasLD" classes actually define mnemonics starting with "st". |
lib/Target/AArch64/AArch64RegisterInfo.td | ||
601–606 ↗ | (On Diff #22386) | I think these should probably go with the existing SubRegIndex definitions. |
608–609 ↗ | (On Diff #22386) | We should probably mention GPRPair here, for readability in "-debug" if nothing else. (Elsewhere too, just because there are no FPRPairs now, doesn't mean there won't be in future). |
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp | ||
4311 ↗ | (On Diff #22386) | We use lower-case "expected". Could also be more detailed here: "expected even general purpose register" for example. |
4316 ↗ | (On Diff #22386) | Space. |
4354–4356 ↗ | (On Diff #22386) | I tend to think you should either trust that the enums are consecutive here or not. Trusting that "x1 == x0 + 1" but then being paranoid over whether maybe "w1 == x0 + 1" seems daft. |
4363 ↗ | (On Diff #22386) | I think things might be a bit simpler in this function overall if you used functions like MCRegisterInfo::getMatchingSuperReg(FirstReg, AArch64::sube0, &PairClass. It could both diagnose an odd input and provide the correct Pair in one step. More generally, you might want to consider a top-level "template<int Width> tryParsePair(...)" which passes the needed 32/64 info (like Pair32RegClass/Pair64Regclass) in as an argument to a non-templated "tryParsePair" function. It's a pattern I've found useful in assembly parsers that have to deal with multiple sizes in the past. |
lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp | ||
1551 ↗ | (On Diff #22386) | Another possible case for a template deferring to the real implementation. These two functions are practically identical. |
lib/Target/AArch64/Disassembler/LLVMBuild.txt | ||
22 ↗ | (On Diff #22386) | I could believe this change is needed, but why does this particular patch reveal the necessity? |
lib/Target/AArch64/InstPrinter/AArch64InstPrinter.cpp | ||
1159 ↗ | (On Diff #22386) | Local variables start with a capital letter (for now). |
Hi Tim, thank you for your hints and comments. I'll address it soon.
Meanwhile shouldn't we perform careful review/re-do/re-base process from the first patch in the dependency chain( http://reviews.llvm.org/D8496 )?
That way I could save efforts rebasing the rest of chain after every correction..
lib/Target/AArch64/Disassembler/LLVMBuild.txt | ||
---|---|---|
22 ↗ | (On Diff #22386) | Introduced usage of AArch64MCRegisterClasses causes the need of this explicit dependency. |
lib/Target/AArch64/AArch64InstrFormats.td | ||
---|---|---|
8767 ↗ | (On Diff #22894) | I'd wish instructions would be better named as "LDST<op>" and "ST<op>".... |
8731–8732 ↗ | (On Diff #22386) | I'd wish instructions would be better named as "LDST<op>" and "ST<op>".... |
lib/Target/AArch64/AArch64RegisterInfo.td | ||
608–609 ↗ | (On Diff #22386) | I'm gonna rename it to WSeqPairs and XSeqPairs, similar to existing DSeqPairs and QSeqPairs |
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp | ||
4331 ↗ | (On Diff #22894) | About top-level template "tryParseGPRSeqPair<>" and generic function: I guess without that knowledge it is possible only to call this generic tryParseGPRSeqPair from tryCustomParseOperand, and it will find out the size of actual operands. //CASP instruction casp x0, x1, x2, x3, [x4] casp w0, w1, w2, w3, [x4] |
4361 ↗ | (On Diff #22894) | No paranoia, second complicated check is needed against pairs like "w0, x1", I've added some tests for that, like //CHECK-ERROR: error: expected consecutive registers of same size //CHECK-ERROR: casp w0, x1, x2, x3, [x5] |
4372 ↗ | (On Diff #22894) | Introduced usage of AArch64MCRegisterClasses causes the need of explicit dependency AArch64Info -> AArch64Desc |
lib/Target/AArch64/Disassembler/LLVMBuild.txt | ||
22 ↗ | (On Diff #22894) | Introduced usage of AArch64MCRegisterClasses causes the need of explicit dependency AArch64Info -> AArch64Desc |
It might seem there are many comments on future actions, while they are all already addressed in last-uploaded diff http://reviews.llvm.org/differential/diff/22894/
So, it is ready to re-review
Looks mostly reasonable. Just a few minor comments now.
Tim.
lib/Target/AArch64/AArch64RegisterInfo.td | ||
---|---|---|
608–609 ↗ | (On Diff #22894) | These lines are longer than the column limit. Not sure about the surrounding ones. |
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp | ||
4322 ↗ | (On Diff #22894) | Debugging code should be removed. |
4332 ↗ | (On Diff #22894) | Since this only ever called for an instruction that *must* have a pair, we can be more helpful in the error messages produced. Always say that we need an even/odd register pair, for example. |
Hi Vladimir,
One more comment...
lib/Target/AArch64/AArch64InstrFormats.td | ||
---|---|---|
8670 ↗ | (On Diff #22894) | Arnold's pointed out to me that it probably should be Rs that gets written here. CASP seems to be right though. |
Hi Arnold,
It seems Tim is unpingable for now...
As soon you have taken a look at this patch, would you be able to raise more comments, or approve the patch?
Ta, Vladimir
Hi Vladimir,
Really sorry I didn't reply to the last two pings; I'd been neglecting the list recently. I think this looks fine now, thanks for updating it.
Tim.