This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add v8.1a atomic instructions
ClosedPublic

Authored by vsukharev on Mar 20 2015, 3:00 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

vsukharev updated this revision to Diff 22386.Mar 20 2015, 3:00 PM
vsukharev retitled this revision from to [AArch64] Add v8.1a atomic instructions.
vsukharev updated this object.
vsukharev edited the test plan for this revision. (Show Details)
vsukharev set the repository for this revision to rL LLVM.
vsukharev added a subscriber: Unknown Object (MLST).

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

vsukharev added inline comments.Mar 24 2015, 5:22 AM
lib/Target/AArch64/Disassembler/LLVMBuild.txt
22 ↗(On Diff #22386)

Introduced usage of AArch64MCRegisterClasses causes the need of this explicit dependency.
Without this change, linker issues an error in shared library mode:
llvm::AArch64MCRegisterClasses could not be found in AArch64Disassembler.cpp:1558 and 1571

vsukharev updated this revision to Diff 22894.Mar 30 2015, 11:52 AM
vsukharev updated this object.

Almost all points are corrected

vsukharev added inline comments.Mar 30 2015, 12:02 PM
lib/Target/AArch64/AArch64InstrFormats.td
8767 ↗(On Diff #22894)

I'd wish instructions would be better named as "LDST<op>" and "ST<op>"....
That way it semantics be clear, so ST just doesn't save loaded value in reg

8731–8732 ↗(On Diff #22386)

I'd wish instructions would be better named as "LDST<op>" and "ST<op>"....
That way it semantics be clear, so ST just doesn't save loaded value in reg

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:
hmmm, is there a way for AArch64AsmParser (methods MatchOperandParserImpl and tryCustomParseOperand in AArch64GenAsmMatcher.inc) to guess in advance which pair to expect - MCK_WSeqPair or MCK_XSeqPair?

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.
I've just tried to use a template and face only tryParseGPRSeqPair<32>() is being called for both test lines, and 64-bit variant is never called,

//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
Without this change, linker issues an error in shared library mode:
llvm::AArch64MCRegisterClasses could not be found in AArch64Disassembler.cpp:1558 and 1571

lib/Target/AArch64/Disassembler/LLVMBuild.txt
22 ↗(On Diff #22894)

Introduced usage of AArch64MCRegisterClasses causes the need of explicit dependency AArch64Info -> AArch64Desc
Without this change, linker issues an error in shared library mode:
llvm::AArch64MCRegisterClasses could not be found in AArch64Disassembler.cpp:1558 and 1571

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

t.p.northover edited edge metadata.Apr 21 2015, 9:59 AM

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.

vsukharev updated this revision to Diff 24210.Apr 22 2015, 4:56 AM
vsukharev edited edge metadata.

Last minor comments are processed.
Thank you for comments and catches.

Hi Tim

Are you happy with Vladimir's final patch?

Ta
Rich

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

t.p.northover accepted this revision.Jun 1 2015, 10:15 AM
t.p.northover edited edge metadata.

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.

This revision is now accepted and ready to land.Jun 1 2015, 10:15 AM
This revision was automatically updated to reflect the committed changes.