On the ARM64 processor, register pushes and pops to/from the stack need to be done with in register pairs using the stp (store pair) and ldp (load pair) instructions. These are needed for saving and restoring the frame pointer register and link register in function prologues and epilogues.
Created attachment 220355 [details] Patch
Comment on attachment 220355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220355&action=review > Source/JavaScriptCore/ChangeLog:14 > + Verified that we corectly generate the new instructions and that they disassemble typo: “corectly” ==> “correctly”. > Source/JavaScriptCore/assembler/ARM64Assembler.h:872 > + enum MemPairOpSize { > + MemPairOp_32, > + MemPairOp_LoadSigned_64, > + MemPairOp_V64 = 1, > + MemPairOp_64, > + MemPairOp_V128 = 2 > + }; I don’t have access to the instruction encoding spec right now, and so I’m uncertain about this. Can you please confirm if MemPairOp_64 is indeed supposed to have the value 2? I could be wrong, but it seems strange to me that the instruction encoding (as used in loadStoreRegisterPairPostIndex() and loadStoreRegisterPairPreIndex()) would have MemPairOp_64 as 2 while MemPairOp_LoadSigned_64 and MemPairOp_V64 are 1. If MemPairOp_64 is supposed to have the value 1 as I suspect, then I suggest expressing the above as: // MemPairOpSize values are computed as (log2(number of bits / 8) - 2)). enum MemPairOpSize { MemPairOp_32 = 0, MemPairOp_LoadSigned_64 = 1, MemPairOp_V64 = 1, MemPairOp_64 = 1, MemPairOp_V128 = 2 }; Spelling out their values explicitly makes it clearer that you intended these enums to have the values of (log2(number of bits / 8) - 2) ... at least clearer than before. I also add the comment indicating that relationship (log2(number of bits / 8) - 2) since the enum is defined here, but the relationship isn’t clear unless we happened to read the implementation of memPairOffsetShift() below. If I’m wrong and MemPairOp_64 is supposed to be 2, then I suggest expressing the enum list as follows: enum MemPairOpSize { MemPairOp_32 = 0, MemPairOp_LoadSigned_64 = 1, MemPairOp_64 = 2, MemPairOp_V64 = 1, MemPairOp_V128 = 2 }; I broke out the V ops from the non-V ops based on "ASSERT(V || (size != MemPairOp_LoadSigned_64) || (opc == MemOp_LOAD))” which I read in loadStoreRegisterPairPostIndex(). IMHO, I think this would better show the relationship between how these enums increment in value. > Source/JavaScriptCore/assembler/ARM64Assembler.h:887 > + static unsigned memPairOffsetShift(bool V, MemPairOpSize size) I presume by “V”, you mean “isVectorMempairOp”. Can you use a more descriptive name like that if the one I suggested is not appropriate? Or maybe add the "“V” means Vector” comment that you use for loadStoreRegisterPairPostIndex() below. > Source/JavaScriptCore/assembler/ARM64Assembler.h:892 > + // return the log2 of the size in bytes, e.g. 64 bit size returns 3 > + if (V) > + return size + 2; > + return (size >> 1) + 2; If MemPairOp_64 is supposed to be 1 instead of 2 (see comment above regarding the MemPairOpSize enum list), then this should reduce to "return size + 2;”. Otherwise, please disregard this comment. > Source/JavaScriptCore/assembler/ARM64Assembler.h:3488 > + ALWAYS_INLINE static int loadStoreRegisterPairPreIndex(MemPairOpSize size, bool V, MemOp opc, int immediate, RegisterID rn, FPRegisterID rt, FPRegisterID rt2) > + { > + ASSERT(size < 3); > + ASSERT(opc == (opc & 1)); // Only load or store, load signed 64 is handled via size. > + ASSERT(V || (size != MemPairOp_LoadSigned_64) || (opc == MemOp_LOAD)); // There isn't an integer store signed. > + unsigned immedShiftAmount = memPairOffsetShift(V, size); > + int imm7 = immediate >> immedShiftAmount; > + ASSERT((imm7 << immedShiftAmount) == immediate && isInt7(imm7)); > + return (0x29800000 | size << 30 | V << 26 | opc << 22 | (imm7 & 0x7f) << 15 | rt2 << 10 | xOrSp(rn) << 5 | rt); > + } This appears to be identical to loadStoreRegisterPairPostIndex() except for a one bit difference in the instruction opcode. I suggest refactoring these 2 functions to be expressed as follows to minimize the amount of cut and paste: ALWAYS_INLINE static int loadStoreRegisterPair(int opcodeBits, MemPairOpSize size, bool V, MemOp opc, int immediate, RegisterID rn, FPRegisterID rt, FPRegisterID rt2) { ... } ALWAYS_INLINE static int loadStoreRegisterPairPostIndex(MemPairOpSize size, bool V, MemOp opc, int immediate, RegisterID rn, FPRegisterID rt, FPRegisterID rt2) { return loadStoreRegisterPair(0x28800000, size, V, opc, immediate, rn, rt, rt2); } ALWAYS_INLINE static int loadStoreRegisterPairPreIndex(MemPairOpSize size, bool V, MemOp opc, int immediate, RegisterID rn, FPRegisterID rt, FPRegisterID rt2) { return loadStoreRegisterPair(0x29800000, size, V, opc, immediate, rn, rt, rt2); } > Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:186 > + Please remove these empty spaces.
(In reply to comment #2) > (From update of attachment 220355 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=220355&action=review > > > Source/JavaScriptCore/ChangeLog:14 > > + Verified that we corectly generate the new instructions and that they disassemble > > typo: “corectly” ==> “correctly”. I will fix. > > Source/JavaScriptCore/assembler/ARM64Assembler.h:872 > > + enum MemPairOpSize { > > + MemPairOp_32, > > + MemPairOp_LoadSigned_64, > > + MemPairOp_V64 = 1, > > + MemPairOp_64, > > + MemPairOp_V128 = 2 > > + }; > > I don’t have access to the instruction encoding spec right now, and so I’m uncertain about this. Can you please confirm if MemPairOp_64 is indeed supposed to have the value 2? I could be wrong, but it seems strange to me that the instruction encoding (as used in loadStoreRegisterPairPostIndex() and loadStoreRegisterPairPreIndex()) would have MemPairOp_64 as 2 while MemPairOp_LoadSigned_64 and MemPairOp_V64 are 1. It is indeed strange, but mostly true. The falsehood is that MemPairOp_LoadSigned_64 should really be MemPairOp_LoadSigned_32 as it loads 32 bit values and then sign extends them. The rest of the names and values match the instruction encodings. Here is a relevant table from the instruction manual (opc is what we call size): Decode fields Instruction Details opc V L 00 0 0 STP — 32-bit 00 0 1 LDP — 32-bit 00 1 0 STP (SIMD&FP) — 32-bit 00 1 1 LDP (SIMD&FP) — 32-bit 01 0 0 UNALLOCATED 01 0 1 LDPSW <= This is Load Pair Signed 01 1 0 STP (SIMD&FP) — 64-bit 01 1 1 LDP (SIMD&FP) — 64-bit 10 0 0 STP — 64-bit 10 0 1 LDP — 64-bit 10 1 0 STP (SIMD&FP) — 128-bit 10 1 1 LDP (SIMD&FP) — 128-bit 11 UNALLOCATED > If MemPairOp_64 is supposed to have the value 1 as I suspect, then I suggest expressing the above as: > > // MemPairOpSize values are computed as (log2(number of bits / 8) - 2)). > enum MemPairOpSize { > MemPairOp_32 = 0, > MemPairOp_LoadSigned_64 = 1, > MemPairOp_V64 = 1, > MemPairOp_64 = 1, > MemPairOp_V128 = 2 > }; > > Spelling out their values explicitly makes it clearer that you intended these enums to have the values of (log2(number of bits / 8) - 2) ... at least clearer than before. I also add the comment indicating that relationship (log2(number of bits / 8) - 2) since the enum is defined here, but the relationship isn’t clear unless we happened to read the implementation of memPairOffsetShift() below. > > If I’m wrong and MemPairOp_64 is supposed to be 2, then I suggest expressing the enum list as follows: > > enum MemPairOpSize { > MemPairOp_32 = 0, > MemPairOp_LoadSigned_64 = 1, > MemPairOp_64 = 2, > > MemPairOp_V64 = 1, > MemPairOp_V128 = 2 > }; I will make these explicit. > I broke out the V ops from the non-V ops based on "ASSERT(V || (size != MemPairOp_LoadSigned_64) || (opc == MemOp_LOAD))” which I read in loadStoreRegisterPairPostIndex(). IMHO, I think this would better show the relationship between how these enums increment in value. This is complicated a little by MemPairOp_32 being the same for scalar and vector registers. > > Source/JavaScriptCore/assembler/ARM64Assembler.h:887 > > + static unsigned memPairOffsetShift(bool V, MemPairOpSize size) > > I presume by “V”, you mean “isVectorMempairOp”. Can you use a more descriptive name like that if the one I suggested is not appropriate? Or maybe add the "“V” means Vector” comment that you use for loadStoreRegisterPairPostIndex() below. The variable name V is used throughout for the V bit in the instruction encodings. It signifies that the instruction is a vector variant. > > Source/JavaScriptCore/assembler/ARM64Assembler.h:892 > > + // return the log2 of the size in bytes, e.g. 64 bit size returns 3 > > + if (V) > > + return size + 2; > > + return (size >> 1) + 2; > > If MemPairOp_64 is supposed to be 1 instead of 2 (see comment above regarding the MemPairOpSize enum list), then this should reduce to "return size + 2;”. Otherwise, please disregard this comment. > > > Source/JavaScriptCore/assembler/ARM64Assembler.h:3488 > > + ALWAYS_INLINE static int loadStoreRegisterPairPreIndex(MemPairOpSize size, bool V, MemOp opc, int immediate, RegisterID rn, FPRegisterID rt, FPRegisterID rt2) > > + { > > + ASSERT(size < 3); > > + ASSERT(opc == (opc & 1)); // Only load or store, load signed 64 is handled via size. > > + ASSERT(V || (size != MemPairOp_LoadSigned_64) || (opc == MemOp_LOAD)); // There isn't an integer store signed. > > + unsigned immedShiftAmount = memPairOffsetShift(V, size); > > + int imm7 = immediate >> immedShiftAmount; > > + ASSERT((imm7 << immedShiftAmount) == immediate && isInt7(imm7)); > > + return (0x29800000 | size << 30 | V << 26 | opc << 22 | (imm7 & 0x7f) << 15 | rt2 << 10 | xOrSp(rn) << 5 | rt); > > + } > > This appears to be identical to loadStoreRegisterPairPostIndex() except for a one bit difference in the instruction opcode. I suggest refactoring these 2 functions to be expressed as follows to minimize the amount of cut and paste: > > ALWAYS_INLINE static int loadStoreRegisterPair(int opcodeBits, MemPairOpSize size, bool V, MemOp opc, int immediate, RegisterID rn, FPRegisterID rt, FPRegisterID rt2) > { > ... > } > > ALWAYS_INLINE static int loadStoreRegisterPairPostIndex(MemPairOpSize size, bool V, MemOp opc, int immediate, RegisterID rn, FPRegisterID rt, FPRegisterID rt2) > { > return loadStoreRegisterPair(0x28800000, size, V, opc, immediate, rn, rt, rt2); > } > > ALWAYS_INLINE static int loadStoreRegisterPairPreIndex(MemPairOpSize size, bool V, MemOp opc, int immediate, RegisterID rn, FPRegisterID rt, FPRegisterID rt2) > { > return loadStoreRegisterPair(0x29800000, size, V, opc, immediate, rn, rt, rt2); > } The function names loadStoreRegisterPairPostIndex() and loadStoreRegisterPairPreIndex() follow the convention in the rest of the file where the function name matches one of the instruction group names. For example there is the group name Load/store register (immediate post-indexed) and the corresponding existing function loadStoreRegisterPostIndex(). I followed the same pattern for the two groups Load/store register pair (post-indexed) and Load/store register pair (pre-indexed). > > Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:186 > > + > > Please remove these empty spaces. I'll remove them.
(In reply to comment #3) > (In reply to comment #2) > > > Source/JavaScriptCore/assembler/ARM64Assembler.h:872 > > > + enum MemPairOpSize { > > > + MemPairOp_32, > > > + MemPairOp_LoadSigned_64, > > > + MemPairOp_V64 = 1, > > > + MemPairOp_64, > > > + MemPairOp_V128 = 2 > > > + }; > > > > I don’t have access to the instruction encoding spec right now, and so I’m uncertain about this. Can you please confirm if MemPairOp_64 is indeed supposed to have the value 2? I could be wrong, but it seems strange to me that the instruction encoding (as used in loadStoreRegisterPairPostIndex() and loadStoreRegisterPairPreIndex()) would have MemPairOp_64 as 2 while MemPairOp_LoadSigned_64 and MemPairOp_V64 are 1. > > It is indeed strange, but mostly true. The falsehood is that MemPairOp_LoadSigned_64 should really be MemPairOp_LoadSigned_32 as it loads 32 bit values and then sign extends them. The rest of the names and values match the instruction encodings. Here is a relevant table from the instruction manual (opc is what we call size): > Decode fields Instruction Details > opc V L > 00 0 0 STP — 32-bit > 00 0 1 LDP — 32-bit > 00 1 0 STP (SIMD&FP) — 32-bit > 00 1 1 LDP (SIMD&FP) — 32-bit > 01 0 0 UNALLOCATED > 01 0 1 LDPSW <= This is Load Pair Signed > 01 1 0 STP (SIMD&FP) — 64-bit > 01 1 1 LDP (SIMD&FP) — 64-bit > 10 0 0 STP — 64-bit > 10 0 1 LDP — 64-bit > 10 1 0 STP (SIMD&FP) — 128-bit > 10 1 1 LDP (SIMD&FP) — 128-bit > 11 UNALLOCATED Funky, but it is what it is. > > If I’m wrong and MemPairOp_64 is supposed to be 2, then I suggest expressing the enum list as follows: > > > > enum MemPairOpSize { > > MemPairOp_32 = 0, > > MemPairOp_LoadSigned_64 = 1, > > MemPairOp_64 = 2, > > > > MemPairOp_V64 = 1, > > MemPairOp_V128 = 2 > > }; > > I will make these explicit. > > > I broke out the V ops from the non-V ops based on "ASSERT(V || (size != MemPairOp_LoadSigned_64) || (opc == MemOp_LOAD))” which I read in loadStoreRegisterPairPostIndex(). IMHO, I think this would better show the relationship between how these enums increment in value. > > This is complicated a little by MemPairOp_32 being the same for scalar and vector registers. In light of the encoding details you provided, I agree that there isn’t an ideal case in terms of grouping. How about: enum MemPairOpSize { MemPairOp_32 = 0, MemPairOp_LoadSigned_64 = 1, MemPairOp_64 = 2, MemPairOp_V32 = MemPairOp_32, MemPairOp_V64 = 1, MemPairOp_V128 = 2 }; That still provides the Vector vs non-Vector distinction while explicitly spelling out that the V32 variant uses the same encoding as the non-Vector 32 bit variant. > The function names loadStoreRegisterPairPostIndex() and loadStoreRegisterPairPreIndex() follow the convention in the rest of the file where the function name matches one of the instruction group names. For example there is the group name Load/store register (immediate post-indexed) and the corresponding > existing function loadStoreRegisterPostIndex(). I followed the same pattern for the two groups Load/store register pair (post-indexed) and Load/store register pair (pre-indexed). Even though there is precedence, I still don’t like the fact that we’re opting for cutting and pasting instead of using a common function. However, granted that these functions are highly unlikely to be mutated in the future (given that the hardware spec that they are based on is also high unlikely to change), I can live with it if no one else objects. With that, I’m satisfied enough with this patch. r=me with agreed changes / fixes applied.
Committed r161318: <http://trac.webkit.org/changeset/161318>