[AArch64] Make getInstSizeInBytes() use instruction size from InstrInfo.td

[tyb0807]

Summary

Currently, AArch64InstrInfo::getInstSizeInBytes() uses hard-coded
instruction size for some pseudo-instructions, while this
information should ideally be found in AArch64InstrInfo.td file (which
can be accessed via MCInstrDesc). Hence, the .td file should be updated
and no hard-coded instruction sizes should be used by
getInstSizeInBytes() anymore.

[tyb0807]

Add tests for SPACE and STATEPOINT pseudo-instructions

[tyb0807]

Update commit message to better describe the context

[lenary]

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

119

I am slighly concerned about losing these comments - could you move them to the tablegen too (as the tablegen also doesn't tell us what they're eventually lowered to)

[nickdesaulniers]

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

97–100

should this case become llvm_unreachable now?

119

+1

Also, perhaps a comment at the top of this switch to prefer setting a Size for new insts in llvm/lib/Target/AArch64/AArch64InstrInfo.td when possible?

[tyb0807]

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

97–100

Well, this is actually handling cases where the fixed constant size is not specified in .td file. I think the intention was to assume that a pseudo-instruction w/o specified size is always expanded to a 4-byte instruction. Is this correct?

[tyb0807]

Add comments about fixed constant size pseudo-instructions in .td file.

[tyb0807]

Move the tablegen querying to the default case to make the code consistent with
ARM version. Update the comment accordingly to clarify the intention of the code

[lenary]

I'm concerned there is no test coverage for:

• SpeculationBarrierISBDSBEndBB and SpeculationBarrierSBEndBB
• StoreSwiftAsyncContext
• JumpTableDest{32,16,8}

[tyb0807]

Add more tests

[lenary]

This was landed accidentally before it was approved, but given @tyb0807 had made the changes I asked, I have not asked him to revert it. I've given Post-commit approval on the specific commit.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

unsigned AArch64InstrInfo::getInstSizeInBytes(const MachineInstr &MI) const {
   // Meta-instructions emit no code.
   if (MI.isMetaInstruction())
     return 0;
 
   // FIXME: We currently only handle pseudoinstructions that don't get expanded
   //        before the assembly printer.
   unsigned NumBytes = 0;
   const MCInstrDesc &Desc = MI.getDesc();
+
+  // Size should be preferably set in
+  // llvm/lib/Target/AArch64/AArch64InstrInfo.td (default case).
+  // Specific cases handle instructions of variable sizes
   switch (Desc.getOpcode()) {
   default:
-    // Anything not explicitly designated otherwise is a normal 4-byte insn.
+    if (Desc.getSize())
+      return Desc.getSize();
+
+    // Anything not explicitly designated otherwise (i.e. pseudo-instructions
+    // with fixed constant size but not specified in .td file) is a normal
+    // 4-byte insn.
     NumBytes = 4;
     break;
   case TargetOpcode::STACKMAP:
     // The upper bound for a stackmap intrinsic is the full length of its shadow
     NumBytes = StackMapOpers(&MI).getNumPatchBytes();
     assert(NumBytes % 4 == 0 && "Invalid number of NOP bytes requested!");
     break;
   case TargetOpcode::PATCHPOINT:
     // The size of the patchpoint intrinsic is the number of bytes requested
     NumBytes = PatchPointOpers(&MI).getNumPatchBytes();
     assert(NumBytes % 4 == 0 && "Invalid number of NOP bytes requested!");
     break;
   case TargetOpcode::STATEPOINT:
     NumBytes = StatepointOpers(&MI).getNumPatchBytes();
     assert(NumBytes % 4 == 0 && "Invalid number of NOP bytes requested!");
     // No patch bytes means a normal call inst is emitted
     if (NumBytes == 0)
       NumBytes = 4;
     break;
-  case AArch64::TLSDESC_CALLSEQ:
-    // This gets lowered to an instruction sequence which takes 16 bytes
-    NumBytes = 16;
-    break;
-  case AArch64::SpeculationBarrierISBDSBEndBB:
-    // This gets lowered to 2 4-byte instructions.
-    NumBytes = 8;
-    break;
-  case AArch64::SpeculationBarrierSBEndBB:
-    // This gets lowered to 1 4-byte instructions.
-    NumBytes = 4;
-    break;
-  case AArch64::JumpTableDest32:
-  case AArch64::JumpTableDest16:
-  case AArch64::JumpTableDest8:
-  case AArch64::MOPSMemoryCopyPseudo:
-  case AArch64::MOPSMemoryMovePseudo:
-  case AArch64::MOPSMemorySetPseudo:
-  case AArch64::MOPSMemorySetTaggingPseudo:
-    NumBytes = 12;
-    break;
   case AArch64::SPACE:
     NumBytes = MI.getOperand(1).getImm();
     break;
-  case AArch64::StoreSwiftAsyncContext:
-    NumBytes = 20;
-    break;
   case TargetOpcode::BUNDLE:
     NumBytes = getInstBundleLength(MI);
     break;
   }
 
   return NumBytes;
 }
 

llvm/lib/Target/AArch64/AArch64InstrInfo.td

def : Pat<(AArch64LOADgot tglobaltlsaddr:$addr),
           (LOADgot tglobaltlsaddr:$addr)>;
 
 def : Pat<(AArch64LOADgot texternalsym:$addr),
           (LOADgot texternalsym:$addr)>;
 
 def : Pat<(AArch64LOADgot tconstpool:$addr),
           (LOADgot tconstpool:$addr)>;
 
+// In general these get lowered into a sequence of three 4-byte instructions.
 // 32-bit jump table destination is actually only 2 instructions since we can
 // use the table itself as a PC-relative base. But optimization occurs after
 // branch relaxation so be pessimistic.
 let Size = 12, Constraints = "@earlyclobber $dst,@earlyclobber $scratch",
     isNotDuplicable = 1 in {
 def JumpTableDest32 : Pseudo<(outs GPR64:$dst, GPR64sp:$scratch),
                              (ins GPR64:$table, GPR64:$entry, i32imm:$jti), []>,
                       Sched<[]>;
def SpeculationSafeValueX
       : Pseudo<(outs GPR64:$dst), (ins GPR64:$src), []>, Sched<[]>;
   def SpeculationSafeValueW
       : Pseudo<(outs GPR32:$dst), (ins GPR32:$src), []>, Sched<[]>;
 }
 
 // SpeculationBarrierEndBB must only be used after an unconditional control
 // flow, i.e. after a terminator for which isBarrier is True.
 let hasSideEffects = 1, isCodeGenOnly = 1, isTerminator = 1, isBarrier = 1 in {
+  // This gets lowered to a pair of 4-byte instructions.
+  let Size = 8 in
   def SpeculationBarrierISBDSBEndBB
       : Pseudo<(outs), (ins), []>, Sched<[]>;
+  // This gets lowered to a 4-byte instruction.
+  let Size = 4 in
   def SpeculationBarrierSBEndBB
       : Pseudo<(outs), (ins), []>, Sched<[]>;
 }
 
 //===----------------------------------------------------------------------===//
 // System instructions.
 //===----------------------------------------------------------------------===//
 

 }
 
 // Pseudo instruction to tell the streamer to emit a 'B' character into the
 // augmentation string.
 def EMITBKEY : Pseudo<(outs), (ins), []>, Sched<[]> {}
 
 // FIXME: maybe the scratch register used shouldn't be fixed to X1?
 // FIXME: can "hasSideEffects be dropped?
-let isCall = 1, Defs = [LR, X0, X1], hasSideEffects = 1,
+// This gets lowered to an instruction sequence which takes 16 bytes
+let isCall = 1, Defs = [LR, X0, X1], hasSideEffects = 1, Size = 16,
     isCodeGenOnly = 1 in
 def TLSDESC_CALLSEQ
     : Pseudo<(outs), (ins i64imm:$sym),
              [(AArch64tlsdesc_callseq tglobaltlsaddr:$sym)]>,
       Sched<[WriteI, WriteLD, WriteI, WriteBrReg]>;
 def : Pat<(AArch64tlsdesc_callseq texternalsym:$sym),
           (TLSDESC_CALLSEQ texternalsym:$sym)>;
 

 // MOPS Node operands: 0: Dst, 1: Src or Value, 2: Size, 3: Chain
 // MOPS Node results: 0: Dst writeback, 1: Size writeback, 2: Chain
 def SDT_AArch64mops : SDTypeProfile<2, 3, [ SDTCisInt<0>, SDTCisInt<1>, SDTCisInt<2> ]>;
 def AArch64mops_memset : SDNode<"AArch64ISD::MOPS_MEMSET", SDT_AArch64mops>;
 def AArch64mops_memset_tagging : SDNode<"AArch64ISD::MOPS_MEMSET_TAGGING", SDT_AArch64mops>;
 def AArch64mops_memcopy : SDNode<"AArch64ISD::MOPS_MEMCOPY", SDT_AArch64mops>;
 def AArch64mops_memmove : SDNode<"AArch64ISD::MOPS_MEMMOVE", SDT_AArch64mops>;
 
+// MOPS operations always contain three 4-byte instructions
 let Predicates = [HasMOPS], Defs = [NZCV], Size = 12, mayStore = 1 in {
   let mayLoad = 1 in {
     def MOPSMemoryCopyPseudo : Pseudo<(outs GPR64common:$Rd_wb, GPR64common:$Rs_wb, GPR64:$Rn_wb),
                                       (ins GPR64common:$Rd, GPR64common:$Rs, GPR64:$Rn),
                                       [], "$Rd = $Rd_wb,$Rs = $Rs_wb,$Rn = $Rn_wb">, Sched<[]>;
     def MOPSMemoryMovePseudo : Pseudo<(outs GPR64common:$Rd_wb, GPR64common:$Rs_wb, GPR64:$Rn_wb),
                                       (ins GPR64common:$Rd, GPR64common:$Rs, GPR64:$Rn),
                                       [], "$Rd = $Rd_wb,$Rs = $Rs_wb,$Rn = $Rn_wb">, Sched<[]>;
   }
   let mayLoad = 0 in {
     def MOPSMemorySetPseudo  : Pseudo<(outs GPR64common:$Rd_wb, GPR64:$Rn_wb),
                                       (ins GPR64common:$Rd, GPR64:$Rn, GPR64:$Rm),
                                       [], "$Rd = $Rd_wb,$Rn = $Rn_wb">, Sched<[]>;
   }
 }
 let Predicates = [HasMOPS, HasMTE], Defs = [NZCV], Size = 12, mayLoad = 0, mayStore = 1 in {
   def MOPSMemorySetTaggingPseudo : Pseudo<(outs GPR64common:$Rd_wb, GPR64:$Rn_wb),
                                           (ins GPR64common:$Rd, GPR64:$Rn, GPR64:$Rm),
                                           [], "$Rd = $Rd_wb,$Rn = $Rn_wb">, Sched<[]>;
 }
 
-let Defs = [X16, X17], mayStore = 1, isCodeGenOnly = 1 in
+// This gets lowered into an instruction sequence of 20 bytes
+let Defs = [X16, X17], mayStore = 1, isCodeGenOnly = 1, Size = 20 in
 def StoreSwiftAsyncContext
       : Pseudo<(outs), (ins GPR64:$ctx, GPR64sp:$base, simm9:$offset),
                []>, Sched<[]>;
 
 def AArch64AssertZExtBool : SDNode<"AArch64ISD::ASSERT_ZEXT_BOOL", SDT_assert>;
 def : Pat<(AArch64AssertZExtBool GPR32:$op),
           (i32 GPR32:$op)>;
 
 include "AArch64InstrAtomics.td"
 include "AArch64SVEInstrInfo.td"
 include "AArch64SMEInstrInfo.td"
 include "AArch64InstrGISel.td"

llvm/unittests/Target/AArch64/InstSizes.cpp

void runChecks(
 
   auto MIRString =
     "--- |\n"
     "  declare void @sizes()\n"
     + InputIRSnippet.str() +
     "...\n"
     "---\n"
     "name: sizes\n"
+    "jumpTable:\n"
+    "  kind:            block-address\n"
+    "  entries:\n"
+    "    - id:              0\n"
+    "      blocks:          [ '%bb.0' ]\n"
     "body: |\n"
     "  bb.0:\n"
     + InputMIRSnippet.str();
 
   std::unique_ptr<MemoryBuffer> MBuffer = MemoryBuffer::getMemBuffer(MIRString);
   std::unique_ptr<MIRParser> MParser =
       createMIRParser(std::move(MBuffer), Context);
   ASSERT_TRUE(MParser);
runChecks(TM.get(), II.get(), "",
             [](AArch64InstrInfo &II, MachineFunction &MF) {
               auto I = MF.begin()->begin();
               EXPECT_EQ(16u, II.getInstSizeInBytes(*I));
               ++I;
               EXPECT_EQ(32u, II.getInstSizeInBytes(*I));
             });
 }
 
+TEST(InstSizes, STATEPOINT) {
+  std::unique_ptr<LLVMTargetMachine> TM = createTargetMachine();
+  std::unique_ptr<AArch64InstrInfo> II = createInstrInfo(TM.get());
+
+  runChecks(TM.get(), II.get(), "",
+            "    STATEPOINT 0, 0, 0, @sizes, 2, 0, 2, 0, 2, 0, 2, 1, 1, 8,"
+            " $sp, 24, 2, 0, 2, 1, 0, 0\n",
+            [](AArch64InstrInfo &II, MachineFunction &MF) {
+              auto I = MF.begin()->begin();
+              EXPECT_EQ(4u, II.getInstSizeInBytes(*I));
+            });
+}
+
+TEST(InstSizes, SPACE) {
+  std::unique_ptr<LLVMTargetMachine> TM = createTargetMachine();
+  std::unique_ptr<AArch64InstrInfo> II = createInstrInfo(TM.get());
+
+  runChecks(TM.get(), II.get(), "",
+            "    $xzr = SPACE 1024, undef $xzr\n"
+            "    dead $xzr = SPACE 4096, $xzr\n",
+            [](AArch64InstrInfo &II, MachineFunction &MF) {
+              auto I = MF.begin()->begin();
+              EXPECT_EQ(1024u, II.getInstSizeInBytes(*I));
+              ++I;
+              EXPECT_EQ(4096u, II.getInstSizeInBytes(*I));
+            });
+}
+
 TEST(InstSizes, TLSDESC_CALLSEQ) {
   std::unique_ptr<LLVMTargetMachine> TM = createTargetMachine();
   std::unique_ptr<AArch64InstrInfo> II = createInstrInfo(TM.get());
 
   runChecks(
       TM.get(), II.get(),
       "  @ThreadLocalGlobal = external thread_local global i32, align 8\n",
       "    TLSDESC_CALLSEQ target-flags(aarch64-tls) @ThreadLocalGlobal\n",
       [](AArch64InstrInfo &II, MachineFunction &MF) {
         auto I = MF.begin()->begin();
         EXPECT_EQ(16u, II.getInstSizeInBytes(*I));
       });
 }
 
-TEST(InstSizes, MOPSMemorySetTaggingPseudo) {
+TEST(InstSizes, StoreSwiftAsyncContext) {
+  std::unique_ptr<LLVMTargetMachine> TM = createTargetMachine();
+  std::unique_ptr<AArch64InstrInfo> II = createInstrInfo(TM.get());
+
+  runChecks(
+      TM.get(), II.get(), "",
+      "    StoreSwiftAsyncContext $x0, $x1, 12, implicit-def $x16, "
+      "implicit-def $x17\n",
+      [](AArch64InstrInfo &II, MachineFunction &MF) {
+        auto I = MF.begin()->begin();
+        EXPECT_EQ(20u, II.getInstSizeInBytes(*I));
+      });
+}
+
+TEST(InstSizes, SpeculationBarrierISBDSBEndBB) {
+  std::unique_ptr<LLVMTargetMachine> TM = createTargetMachine();
+  std::unique_ptr<AArch64InstrInfo> II = createInstrInfo(TM.get());
+
+  runChecks(
+      TM.get(), II.get(), "",
+      "    SpeculationBarrierISBDSBEndBB\n"
+      "    BR $x8\n",
+      [](AArch64InstrInfo &II, MachineFunction &MF) {
+        auto I = MF.begin()->begin();
+        EXPECT_EQ(8u, II.getInstSizeInBytes(*I));
+      });
+}
+
+TEST(InstSizes, SpeculationBarrierSBEndBB) {
+  std::unique_ptr<LLVMTargetMachine> TM = createTargetMachine();
+  std::unique_ptr<AArch64InstrInfo> II = createInstrInfo(TM.get());
+
+  runChecks(
+      TM.get(), II.get(), "",
+      "    SpeculationBarrierSBEndBB\n"
+      "    BR $x8\n",
+      [](AArch64InstrInfo &II, MachineFunction &MF) {
+        auto I = MF.begin()->begin();
+        EXPECT_EQ(4u, II.getInstSizeInBytes(*I));
+      });
+}
+
+TEST(InstSizes, JumpTable) {
+  std::unique_ptr<LLVMTargetMachine> TM = createTargetMachine();
+  std::unique_ptr<AArch64InstrInfo> II = createInstrInfo(TM.get());
+
+  runChecks(TM.get(), II.get(), "",
+            "    $x10, $x11 = JumpTableDest32 $x9, $x8, %jump-table.0\n"
+            "    $x10, $x11 = JumpTableDest16 $x9, $x8, %jump-table.0\n"
+            "    $x10, $x11 = JumpTableDest8 $x9, $x8, %jump-table.0\n",
+            [](AArch64InstrInfo &II, MachineFunction &MF) {
+              auto I = MF.begin()->begin();
+              EXPECT_EQ(12u, II.getInstSizeInBytes(*I));
+              ++I;
+              EXPECT_EQ(12u, II.getInstSizeInBytes(*I));
+              ++I;
+              EXPECT_EQ(12u, II.getInstSizeInBytes(*I));
+            });
+}
+
+TEST(InstSizes, MOPSMemoryPseudos) {
   std::unique_ptr<LLVMTargetMachine> TM = createTargetMachine();
   std::unique_ptr<AArch64InstrInfo> II = createInstrInfo(TM.get());
 
   runChecks(TM.get(), II.get(), "",
-            "  renamable $x0, dead renamable $x1 = MOPSMemorySetTaggingPseudo "
-            "killed renamable $x0, killed renamable $x1, killed renamable $x2, "
-            "implicit-def dead $nzcv\n",
+            "  $x0, $x1, $x2 = MOPSMemoryMovePseudo $x0, $x1, $x2, "
+            "implicit-def $nzcv\n"
+            "  $x0, $x1 = MOPSMemorySetPseudo $x0, $x1, $x2, "
+            "implicit-def $nzcv\n"
+            "  $x0, $x1, $x8 = MOPSMemoryCopyPseudo $x0, $x1, $x8, "
+            "implicit-def $nzcv\n"
+            "  $x0, $x1 = MOPSMemorySetTaggingPseudo $x0, $x1, $x2, "
+            "implicit-def $nzcv\n",
             [](AArch64InstrInfo &II, MachineFunction &MF) {
               auto I = MF.begin()->begin();
               EXPECT_EQ(12u, II.getInstSizeInBytes(*I));
+              ++I;
+              EXPECT_EQ(12u, II.getInstSizeInBytes(*I));
+              ++I;
+              EXPECT_EQ(12u, II.getInstSizeInBytes(*I));
+              ++I;
+              EXPECT_EQ(12u, II.getInstSizeInBytes(*I));
             });
 }