Add AArch64 unit tests

[rovka]

Summary

Add unit tests for checking a few tricky instruction sizes. Also remove the old
tests for the instruction sizes, which were clunky and brittle.

Since this is the first set of target-specific unit tests, we need to add some
CMake plumbing. In the future, adding unit tests for a given target will be as
simple as creating a directory with the same name as the target under
unittests/Target. The tests are only run if the target is enabled in
LLVM_TARGETS_TO_BUILD.

[rengolin]

Hi Diana,

Nice usage of lambdas.

I'm guessing that the MIR snippet you have in runChecks() is the bare minimum to make it parse, in which case, any sequence of machine instructions would work with that pattern. For now, that being inside an anonymous namespace in the AArch64 part of the tests is ok, but if more targets start using this pattern (which I think it's a good thing), we should be moving this up somewhere. Not in this patch, but maybe a comment would be good.

Overall, LGTM, but I'll let someone else have a look, too.

cheers,
--renato

unittests/Target/AArch64/InstSizes.cpp

88 ↗

(On Diff #71307)

why the extra space?

[rovka]

Hi. Thanks for looking :)

In D24548#545770, @rengolin wrote:

I'm guessing that the MIR snippet you have in runChecks() is the bare minimum to make it parse, in which case, any sequence of machine instructions would work with that pattern. For now, that being inside an anonymous namespace in the AArch64 part of the tests is ok, but if more targets start using this pattern (which I think it's a good thing), we should be moving this up somewhere. Not in this patch, but maybe a comment would be good.

It's the bare minimum to run these 3 tests, and it's probably good enough for any instruction-size tests. It used to be even smaller before I had to add the global for the TLS test, so it's not impossible for other tests to have other needs as well (e.g. tests that need more than one basic block or more than one function would probably rather not have this particular MIR stub). I think it needs to evolve a bit before being extracted. I'll try to think of a comment along those lines.

unittests/Target/AArch64/InstSizes.cpp

88 ↗

(On Diff #71307)

Erm, it just felt like a natural way to separate the 2 checks. But I guess with everything else so compact it may look a bit awkward. I'll remove it before committing.

[rovka]

Address Renato's comments.

[rovka]

Rebased. Also ping.

[MatzeB]

This test looks good to me.

In general though these programmatic unit tests are harder to understand and more work to maintain than (well done) tests using one of the llvm tool. Of course to do this effectively we need a means to print/test the information/functionality we are interested in and right now there is no way to print the computed size next to each instruction or similar. I am also aware that I did a similar thing in MI/LiveIntervalTest.cpp because there was no good way to force codegen to move exactly 1 instruction to another place, so I guess this is more of a "kids don't do this at home" warning :)

unittests/Target/AArch64/CMakeLists.txt

6–7 ↗

(On Diff #73585)

Doesn't sound so bad to me that it warrants a "FIXME".

[rovka]

In D24548#567443, @MatzeB wrote:

In general though these programmatic unit tests are harder to understand and more work to maintain than (well done) tests using one of the llvm tool. Of course to do this effectively we need a means to print/test the information/functionality we are interested in and right now there is no way to print the computed size next to each instruction or similar.

Would it be ok to add a pass that annotates the MIR with the instruction sizes (as comments) just so we can have the usual kind of tests with FileCheck? Just wondering, I'm definitely leaving that for the next person that has to deal with an instruction-size related bug :)

Thanks for reviewing.

[MatzeB]

In D24548#568011, @rovka wrote:

In D24548#567443, @MatzeB wrote:

In general though these programmatic unit tests are harder to understand and more work to maintain than (well done) tests using one of the llvm tool. Of course to do this effectively we need a means to print/test the information/functionality we are interested in and right now there is no way to print the computed size next to each instruction or similar.

Would it be ok to add a pass that annotates the MIR with the instruction sizes (as comments) just so we can have the usual kind of tests with FileCheck? Just wondering, I'm definitely leaving that for the next person that has to deal with an instruction-size related bug :)

Yes I would imagine something in the line of adding a cl::opt flag (restricted to assert builds) that either annotates .mir output or assembly output with comments, or create a new pass that prints the debug information you need (though for this specific case a whole new pass seems a bit much).

llvm/trunk/test/CodeGen/MIR/AArch64/inst-size-patchpoint.mir

-# RUN: llc -mtriple=aarch64-unknown -run-pass branch-relaxation -aarch64-tbz-offset-bits=4 %s -o - | FileCheck %s
---- |
-  ; ModuleID = '/tmp/test.ll'
-  source_filename = "test.ll"
-  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
-  target triple = "aarch64-unknown"
-
-  define void @test_patchpoint_length(i32 %in) {
-    %val = and i32 %in, 1
-    %tst = icmp eq i32 %val, 0
-    br i1 %tst, label %true, label %false
-
-  true:                                             ; preds = %0
-    tail call void (i64, i32, i8*, i32, ...) @llvm.experimental.patchpoint.void(i64 0, i32 32, i8* null, i32 0)
-    ret void
-
-  false:                                            ; preds = %0
-    ret void
-  }
-
-  declare void @llvm.experimental.patchpoint.void(i64, i32, i8*, i32, ...)
-...
----
-# CHECK-LABEL: name:{{.*}}test_patchpoint_length
-# If the size of the patchpoint is computed correctly, that will push
-# the bb.2.false block too far away from the TBNZW, so the branch will
-# have to be relaxed (note that we're using -aarch64-tbz-offset-bits to
-# constrain the range that can be reached with the TBNZW to something smaller
-# than what the patchpoint is lowered to).
-# CHECK: TBZW killed %w0, 0, %bb.1.true
-# CHECK: B %bb.2.false
-name:            test_patchpoint_length
-tracksRegLiveness: false
-liveins:
-  - { reg: '%w0' }
-frameInfo:
-  hasPatchPoint:   true
-  stackSize:       16
-  offsetAdjustment: 0
-  maxAlignment:    8
-  adjustsStack:    true
-  hasCalls:        true
-  maxCallFrameSize: 0
-stack:
-  - { id: 0, type: spill-slot, offset: -8, size: 8, alignment: 8, callee-saved-register: '%lr' }
-  - { id: 1, type: spill-slot, offset: -16, size: 8, alignment: 8, callee-saved-register: '%fp' }
-body:             |
-  bb.0 (%ir-block.0):
-    successors: %bb.1.true, %bb.2.false
-    liveins: %w0, %lr, %fp
-
-    TBNZW killed %w0, 0, %bb.2.false
-
-  bb.1.true:
-    successors: %bb.2.false
-    liveins: %lr, %fp
-
-    PATCHPOINT 0, 32, 0, 0, 0, csr_aarch64_aapcs, implicit-def dead early-clobber %x16, implicit-def dead early-clobber %x17, implicit-def dead early-clobber %lr, implicit-def %sp
-
-  bb.2.false:
-    liveins: %lr, %fp
-
-    RET killed %lr
-
-...

llvm/trunk/test/CodeGen/MIR/AArch64/inst-size-stackmap.mir

-# RUN: llc -mtriple=aarch64-unknown -run-pass branch-relaxation -aarch64-tbz-offset-bits=4 %s -o - | FileCheck %s
---- |
-  ; ModuleID = 'test.ll'
-  source_filename = "test.ll"
-  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
-  target triple = "aarch64-unknown"
-
-  define void @test_stackmap_length(i32 %in) {
-    %val = and i32 %in, 1
-    %tst = icmp eq i32 %val, 0
-    br i1 %tst, label %true, label %false
-
-  true:                                             ; preds = %0
-    tail call void (i64, i32, ...) @llvm.experimental.stackmap(i64 0, i32 32)
-    ret void
-
-  false:                                            ; preds = %0
-    ret void
-  }
-
-  declare void @llvm.experimental.stackmap(i64, i32, ...)
-...
----
-# CHECK-LABEL: name:{{.*}}test_stackmap_length
-# If the size of the stackmap is computed correctly, that will push
-# the bb.2.false block too far away from the TBNZW, so the branch will
-# have to be relaxed (note that we're using -aarch64-tbz-offset-bits to
-# constrain the range that can be reached with the TBNZW to something smaller
-# than what the stackmap is lowered to).
-# CHECK: TBZW killed %w0, 0, %bb.1.true
-# CHECK: B %bb.2.false
-name:            test_stackmap_length
-tracksRegLiveness: false
-liveins:
-  - { reg: '%w0' }
-frameInfo:
-  hasStackMap:     true
-  stackSize:       16
-  offsetAdjustment: 0
-  maxAlignment:    8
-  adjustsStack:    true
-  hasCalls:        true
-stack:
-  - { id: 0, type: spill-slot, offset: -8, size: 8, alignment: 8, callee-saved-register: '%lr' }
-  - { id: 1, type: spill-slot, offset: -16, size: 8, alignment: 8, callee-saved-register: '%fp' }
-body:             |
-  bb.0 (%ir-block.0):
-    successors: %bb.1.true, %bb.2.false
-    liveins: %w0, %lr, %fp
-
-    TBNZW killed %w0, 0, %bb.2.false
-
-  bb.1.true:
-    successors: %bb.2.false
-    liveins: %lr, %fp
-
-    STACKMAP 0, 32, implicit-def dead early-clobber %x16, implicit-def dead early-clobber %x17, implicit-def dead early-clobber %lr
-
-  bb.2.false:
-    liveins: %lr, %fp
-
-    RET killed %lr
-

llvm/trunk/test/CodeGen/MIR/AArch64/inst-size-tlsdesc-callseq.mir

-# RUN: llc -mtriple=aarch64-unknown -run-pass branch-relaxation -aarch64-tbz-offset-bits=4 %s -o - | FileCheck %s
---- |
-  ; ModuleID = 'test.ll'
-  source_filename = "test.ll"
-  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
-  target triple = "aarch64-unknown"
-
-  @ThreadLocalGlobal = external thread_local local_unnamed_addr global i32, align 8
-
-  define i32 @test_tlsdesc_callseq_length(i32 %in) {
-    %val = and i32 %in, 1
-    %tst = icmp eq i32 %val, 0
-    br i1 %tst, label %true, label %false
-
-  true:                                             ; preds = %0
-    %1 = load i32, i32* @ThreadLocalGlobal, align 8
-    ret i32 %1
-
-  false:                                            ; preds = %0
-    ret i32 0
-  }
-
-...
----
-# CHECK-LABEL: name:{{.*}}test_tlsdesc_callseq_length
-# If the size of TLSDESC_CALLSEQ is computed correctly, that will push
-# the bb.2.false block too far away from the TBNZW, so the branch will
-# have to be relaxed (note that we're using -aarch64-tbz-offset-bits to
-# constrain the range that can be reached with the TBNZW to something smaller
-# than what TLSDESC_CALLSEQ is lowered to).
-# CHECK: TBZW killed %w0, 0, %bb.1.true
-# CHECK: B %bb.2.false
-name:            test_tlsdesc_callseq_length
-alignment:       2
-exposesReturnsTwice: false
-tracksRegLiveness: false
-liveins:
-  - { reg: '%w0' }
-frameInfo:
-  isFrameAddressTaken: false
-  isReturnAddressTaken: false
-  hasStackMap:     false
-  hasPatchPoint:   false
-  stackSize:       16
-  offsetAdjustment: 0
-  maxAlignment:    16
-  adjustsStack:    false
-  hasCalls:        true
-  maxCallFrameSize: 0
-  hasOpaqueSPAdjustment: false
-  hasVAStart:      false
-  hasMustTailInVarArgFunc: false
-stack:
-  - { id: 0, type: spill-slot, offset: -16, size: 8, alignment: 16, callee-saved-register: '%lr' }
-body:             |
-  bb.0 (%ir-block.0):
-    successors: %bb.1.true, %bb.2.false
-    liveins: %w0, %lr
-
-    TBNZW killed %w0, 0, %bb.2.false
-
-  bb.1.true:
-    liveins: %lr
-
-    early-clobber %sp = frame-setup STRXpre killed %lr, %sp, -16 :: (store 8 into %stack.0)
-    frame-setup CFI_INSTRUCTION def_cfa_offset 16
-    frame-setup CFI_INSTRUCTION offset %w30, -16
-    TLSDESC_CALLSEQ target-flags(aarch64-tls) @ThreadLocalGlobal, implicit-def dead %lr, implicit-def %x0, implicit-def dead %x1
-    %x8 = MRS 56962
-    %w0 = LDRWroX killed %x8, killed %x0, 0, 0 :: (load 4 from @ThreadLocalGlobal, align 8)
-    early-clobber %sp, %lr = LDRXpost %sp, 16 :: (load 8 from %stack.0)
-    RET killed %lr, implicit killed %w0
-
-  bb.2.false:
-    liveins: %lr
-
-    %w0 = ORRWrs %wzr, %wzr, 0
-    RET killed %lr, implicit killed %w0
-
-...

llvm/trunk/unittests/CMakeLists.txt

 add_subdirectory(LineEditor)
 add_subdirectory(Linker)
 add_subdirectory(MC)
 add_subdirectory(MI)
 add_subdirectory(ObjectYAML)
 add_subdirectory(Option)
 add_subdirectory(ProfileData)
 add_subdirectory(Support)
+add_subdirectory(Target)
 add_subdirectory(Transforms)

llvm/trunk/unittests/Target/AArch64/CMakeLists.txt

+include_directories(
+  ${CMAKE_SOURCE_DIR}/lib/Target/AArch64
+  ${CMAKE_BINARY_DIR}/lib/Target/AArch64
+  )
+
+# FIXME: We're really requiring way too many components here, and a lot of it is
+# because of the TargetOptions
+set(LLVM_LINK_COMPONENTS
+  AArch64CodeGen
+  AArch64Desc
+  AArch64Info
+  CodeGen
+  Core
+  MC
+  MIRParser
+  SelectionDAG
+  Support
+  Target
+  )
+
+add_llvm_unittest(AArch64Tests
+  InstSizes.cpp
+  )

llvm/trunk/unittests/Target/AArch64/InstSizes.cpp

+#include "AArch64Subtarget.h"
+#include "AArch64TargetMachine.h"
+#include "llvm/CodeGen/MIRParser/MIRParser.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+std::unique_ptr<TargetMachine> createTargetMachine() {
+  auto TT(Triple::normalize("aarch64--"));
+  std::string CPU("generic");
+  std::string FS("");
+
+  LLVMInitializeAArch64TargetInfo();
+  LLVMInitializeAArch64Target();
+  LLVMInitializeAArch64TargetMC();
+
+  std::string Error;
+  const Target *TheTarget = TargetRegistry::lookupTarget(TT, Error);
+  assert(TheTarget && "Target not registered");
+
+  return std::unique_ptr<TargetMachine>(
+      TheTarget->createTargetMachine(TT, CPU, FS, TargetOptions(), None,
+                                     CodeModel::Default, CodeGenOpt::Default));
+}
+
+std::unique_ptr<AArch64InstrInfo> createInstrInfo(TargetMachine *TM) {
+  AArch64Subtarget ST(TM->getTargetTriple(), TM->getTargetCPU(),
+                      TM->getTargetFeatureString(), *TM, /* isLittle */ false);
+  return llvm::make_unique<AArch64InstrInfo>(ST);
+}
+
+/// The \p InputIRSnippet is only needed for things that can't be expressed in
+/// the \p InputMIRSnippet (global variables etc)
+/// TODO: Some of this might be useful for other architectures as well - extract
+///       the platform-independent parts somewhere they can be reused.
+void runChecks(
+    TargetMachine *TM, AArch64InstrInfo *II, const StringRef InputIRSnippet,
+    const StringRef InputMIRSnippet,
+    std::function<void(AArch64InstrInfo &, MachineFunction &)> Checks) {
+  LLVMContext Context;
+
+  auto MIRString =
+    "--- |\n"
+    "  declare void @sizes()\n"
+    + InputIRSnippet.str() +
+    "...\n"
+    "---\n"
+    "name: sizes\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(MParser && "Couldn't create MIR parser");
+
+  std::unique_ptr<Module> M = MParser->parseLLVMModule();
+  assert(M && "Couldn't parse module");
+
+  M->setTargetTriple(TM->getTargetTriple().getTriple());
+  M->setDataLayout(TM->createDataLayout());
+
+  auto F = M->getFunction("sizes");
+  assert(F && "Couldn't find intended function");
+
+  MachineModuleInfo MMI(TM);
+  MMI.setMachineFunctionInitializer(MParser.get());
+  auto &MF = MMI.getMachineFunction(*F);
+
+  Checks(*II, MF);
+}
+
+} // anonymous namespace
+
+TEST(InstSizes, STACKMAP) {
+  std::unique_ptr<TargetMachine> TM = createTargetMachine();
+  std::unique_ptr<AArch64InstrInfo> II = createInstrInfo(TM.get());
+
+  runChecks(TM.get(), II.get(), "", "    STACKMAP 0, 16\n"
+                                    "    STACKMAP 1, 32\n",
+            [](AArch64InstrInfo &II, MachineFunction &MF) {
+              auto I = MF.begin()->begin();
+              EXPECT_EQ(16u, II.getInstSizeInBytes(*I));
+              ++I;
+              EXPECT_EQ(32u, II.getInstSizeInBytes(*I));
+            });
+}
+
+TEST(InstSizes, PATCHPOINT) {
+  std::unique_ptr<TargetMachine> TM = createTargetMachine();
+  std::unique_ptr<AArch64InstrInfo> II = createInstrInfo(TM.get());
+
+  runChecks(TM.get(), II.get(), "",
+            "    PATCHPOINT 0, 16, 0, 0, 0, csr_aarch64_aapcs\n"
+            "    PATCHPOINT 1, 32, 0, 0, 0, csr_aarch64_aapcs\n",
+            [](AArch64InstrInfo &II, MachineFunction &MF) {
+              auto I = MF.begin()->begin();
+              EXPECT_EQ(16u, II.getInstSizeInBytes(*I));
+              ++I;
+              EXPECT_EQ(32u, II.getInstSizeInBytes(*I));
+            });
+}
+
+TEST(InstSizes, TLSDESC_CALLSEQ) {
+  std::unique_ptr<TargetMachine> 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));
+      });
+}

llvm/trunk/unittests/Target/CMakeLists.txt

+foreach(t ${LLVM_TARGETS_TO_BUILD})
+  if(IS_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/${t})
+    add_subdirectory(${t})
+  endif()
+endforeach()