[PATCH] [AArch64] Add v8.1a atomic instructions
Tim Northover
t.p.northover at gmail.com
Fri Mar 20 20:04:36 PDT 2015
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.
REPOSITORY
rL LLVM
================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:8633-8636
@@ -8632,2 +8632,6 @@
def : TokenAlias<".D", ".d">;
def : TokenAlias<".Q", ".q">;
+
+//----------------------------------------------------------------------------
+// v8.1 atomic instructions extension:
+// * CAS
----------------
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.
================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:8668-8669
@@ +8667,4 @@
+ bit NP;
+ bit A;
+ bit R;
+ bits<5> Rs;
----------------
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.
================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:8685-8690
@@ +8684,8 @@
+
+class BaseCAS<string order, string size, RegisterClass RC>
+ : BaseCASEncoding<(outs RC:$out),(ins RC:$Rs, RC:$Rt, GPR64sp:$Rn),
+ "cas" # order # size, "\t$Rs, $Rt, [$Rn]",
+ "$out = $Rt",[]> {
+ let NP = 1;
+}
+
----------------
You should probably put BaseCAS and BaseCASP together. For a long while I thought this whole indirection was pointless (until I saw CASP).
================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:8693
@@ +8692,3 @@
+let Predicates = [HasV8_1a], mayLoad = 1, mayStore = 1, hasSideEffects = 1 in
+class BaseLD<string op, string order, string size, RegisterClass RC>
+ : I<(outs RC:$Rt),(ins RC:$Rs, GPR64sp:$Rn), "ld" # op # order # size, "\t$Rs, $Rt, [$Rn]","",[]> {
----------------
"BaseLD" is far too generic as a name for something that's only applicable to these atomicrmw operations.
================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:8715-8716
@@ +8714,4 @@
+
+multiclass LDOp <string order, string size, RegisterClass RC> {
+ let opc = 0b000 in def _add : BaseLD<"add" , order, size, RC>;
+ let opc = 0b001 in def _clr : BaseLD<"clr" , order, size, RC>;
----------------
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).
================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:8731-8732
@@ +8730,4 @@
+
+multiclass BaseAliasLDOp<string asm> {
+ def : BaseAliasLD<"st" # asm # "lb", GPR32, WZR, !cast<Instruction>("LD" # _B_release_ # asm)>;
+ def : BaseAliasLD<"st" # asm # "lh", GPR32, WZR, !cast<Instruction>("LD" # _H_release_ # asm)>;
----------------
It's also rather odd that the "BaseAliasLD" classes actually define mnemonics starting with "st".
================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.td:601-606
@@ +600,8 @@
+
+let Namespace = "AArch64" in {
+ def sube32 : SubRegIndex<32>;
+ def subo32 : SubRegIndex<32>;
+ def sube64 : SubRegIndex<64>;
+ def subo64 : SubRegIndex<64>;
+}
+
----------------
I think these should probably go with the existing SubRegIndex definitions.
================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.td:608-609
@@ +607,4 @@
+
+def Pair32 : RegisterTuples<[sube32, subo32], [(rotl GPR32, 0), (rotl GPR32, 1)]>;
+def Pair64 : RegisterTuples<[sube64, subo64], [(rotl GPR64, 0), (rotl GPR64, 1)]>;
+
----------------
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).
================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:4311
@@ +4310,3 @@
+ if (getParser().getTok().isNot(AsmToken::Identifier)) {
+ Error(S, "Expected register");
+ return MatchOperand_ParseFail;
----------------
We use lower-case "expected". Could also be more detailed here: "expected even general purpose register" for example.
================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:4316
@@ +4315,3 @@
+ int FirstReg = tryParseRegister();
+ if (FirstReg ==-1) {
+ return MatchOperand_ParseFail;
----------------
Space.
================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:4354-4356
@@ +4353,5 @@
+
+ if (RI->getEncodingValue(SecondReg) != FirstEncoding + 1 ||
+ (isXReg && !XRegClass.contains(SecondReg)) ||
+ (isWReg && !WRegClass.contains(SecondReg))) {
+ Error(E,"Expected consecutive registers");
----------------
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.
================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:4363
@@ +4362,3 @@
+ if (isXReg) {
+ Pair = AArch64MCRegisterClasses[AArch64::Pair64ClassRegClassID].getRegister(
+ FirstEncoding);
----------------
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.
================
Comment at: lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp:1551
@@ +1550,3 @@
+
+static DecodeStatus DecodePair32ClassRegisterClass(MCInst &Inst, unsigned RegNo,
+ uint64_t Addr,
----------------
Another possible case for a template deferring to the real implementation. These two functions are practically identical.
================
Comment at: lib/Target/AArch64/Disassembler/LLVMBuild.txt:22
@@ -21,3 +21,3 @@
parent = AArch64
-required_libraries = AArch64Info AArch64Utils MC MCDisassembler Support
+required_libraries = AArch64Desc AArch64Info AArch64Utils MC MCDisassembler Support
add_to_library_groups = AArch64
----------------
I could believe this change is needed, but why does this particular patch reveal the necessity?
================
Comment at: lib/Target/AArch64/InstPrinter/AArch64InstPrinter.cpp:1159
@@ +1158,3 @@
+
+ unsigned sube = (size == 32) ? AArch64::sube32 : AArch64::sube64;
+ unsigned subo = (size == 32) ? AArch64::subo32 : AArch64::subo64;
----------------
Local variables start with a capital letter (for now).
http://reviews.llvm.org/D8501
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list