Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISCV][test] Add tests for RISCVInstrInfo::describeLoadedValue #76041

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

asb
Copy link
Contributor

@asb asb commented Dec 20, 2023

Tests are in preparation for adding handling of the load of a constant value as Mips does (noted in
#72356 (comment)).

I've opted to implement these tests as a C++ unit test as on balance I think it's easier to follow and maintain than .mir tests trying to indirectly test this function. That said, you see the limitations with the test of describeLoadedValue on a memory operation where we'd rather pass MachinePointerInfo::getFixedStack but can't because we'd need to then ensure the necessary stack metadata for the function is present.

Tests are in preparation for adding handling of the load of a constant
value as Mips does (noted in
<llvm#72356 (comment)>).

I've opted to implement these tests as a C++ unit test as on balance I
_think_ it's easier to follow and maintain than .mir tests trying to
indirectly test this function. That said, you see the limitations with
the test of describeLoadedValue on a memory operation where we'd rather
pass `MachinePointerInfo::getFixedStack` but can't because we'd need to
then ensure the necessary stack metadata for the function is present.
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 20, 2023

@llvm/pr-subscribers-backend-risc-v

Author: Alex Bradbury (asb)

Changes

Tests are in preparation for adding handling of the load of a constant value as Mips does (noted in
<#72356 (comment)>).

I've opted to implement these tests as a C++ unit test as on balance I think it's easier to follow and maintain than .mir tests trying to indirectly test this function. That said, you see the limitations with the test of describeLoadedValue on a memory operation where we'd rather pass MachinePointerInfo::getFixedStack but can't because we'd need to then ensure the necessary stack metadata for the function is present.


Full diff: https://github.com/llvm/llvm-project/pull/76041.diff

1 Files Affected:

  • (modified) llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp (+78)
diff --git a/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
index 5ef86bb3f7b46e..5836239bc56fd6 100644
--- a/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
+++ b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
@@ -10,6 +10,7 @@
 #include "RISCVSubtarget.h"
 #include "RISCVTargetMachine.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Target/TargetLoweringObjectFile.h"
@@ -174,6 +175,83 @@ TEST_P(RISCVInstrInfoTest, GetMemOperandsWithOffsetWidth) {
   EXPECT_EQ(Width, 4u);
 }
 
+static void expectDIEPrintResult(const DIExpression *Expr, StringRef Expected) {
+  std::string Output;
+  raw_string_ostream OS(Output);
+  Expr->print(OS);
+  OS.flush();
+  EXPECT_EQ(OS.str(), Expected);
+}
+
+TEST_P(RISCVInstrInfoTest, DescribeLoadedValue) {
+  const RISCVInstrInfo *TII = ST->getInstrInfo();
+  DebugLoc DL;
+
+  MachineBasicBlock *MBB = MF->CreateMachineBasicBlock();
+  MF->getProperties().set(MachineFunctionProperties::Property::NoVRegs);
+
+  // Register move.
+  auto *MI1 = BuildMI(*MBB, MBB->begin(), DL, TII->get(RISCV::ADDI), RISCV::X1)
+                  .addReg(RISCV::X2)
+                  .addImm(0)
+                  .getInstr();
+  EXPECT_FALSE(TII->describeLoadedValue(*MI1, RISCV::X2).has_value());
+  std::optional<ParamLoadedValue> MI1Res =
+      TII->describeLoadedValue(*MI1, RISCV::X1);
+  ASSERT_TRUE(MI1Res.has_value());
+  ASSERT_TRUE(MI1Res->first.isReg());
+  EXPECT_EQ(MI1Res->first.getReg(), RISCV::X2);
+  expectDIEPrintResult(MI1Res->second, "!DIExpression()");
+
+  // Load immediate.
+  auto *MI2 = BuildMI(*MBB, MBB->begin(), DL, TII->get(RISCV::ADDI), RISCV::X3)
+                  .addReg(RISCV::X0)
+                  .addImm(111)
+                  .getInstr();
+  std::optional<ParamLoadedValue> MI2Res =
+      TII->describeLoadedValue(*MI2, RISCV::X3);
+  ASSERT_TRUE(MI2Res.has_value());
+  ASSERT_TRUE(MI2Res->first.isReg());
+  EXPECT_EQ(MI2Res->first.getReg(), RISCV::X0);
+  // TODO: Could be a DW_OP_constu if this is recognised as a immediate load
+  // rather than just an addi.
+  expectDIEPrintResult(MI2Res->second, "!DIExpression(DW_OP_plus_uconst, 111)");
+
+  // Add immediate.
+  auto *MI3 = BuildMI(*MBB, MBB->begin(), DL, TII->get(RISCV::ADDI), RISCV::X2)
+                  .addReg(RISCV::X3)
+                  .addImm(222)
+                  .getInstr();
+  std::optional<ParamLoadedValue> MI3Res =
+      TII->describeLoadedValue(*MI3, RISCV::X2);
+  ASSERT_TRUE(MI3Res.has_value());
+  ASSERT_TRUE(MI3Res->first.isReg());
+  EXPECT_EQ(MI3Res->first.getReg(), RISCV::X3);
+  expectDIEPrintResult(MI3Res->second, "!DIExpression(DW_OP_plus_uconst, 222)");
+
+  // Load value from memory.
+  // It would be better (more reflective of real-world describeLoadedValue
+  // usage) to test using MachinePointerInfo::getFixedStack, but
+  // unfortunately it would be overly fiddly to make this work.
+  auto MMO = MF->getMachineMemOperand(MachinePointerInfo::getConstantPool(*MF),
+                                      MachineMemOperand::MOLoad, 1, Align(1));
+  auto *MI4 = BuildMI(*MBB, MBB->begin(), DL, TII->get(RISCV::LB), RISCV::X1)
+                  .addReg(RISCV::X2)
+                  .addImm(-128)
+                  .addMemOperand(MMO)
+                  .getInstr();
+  std::optional<ParamLoadedValue> MI4Res =
+      TII->describeLoadedValue(*MI4, RISCV::X1);
+  ASSERT_TRUE(MI4Res.has_value());
+  ASSERT_TRUE(MI4Res->first.isReg());
+  EXPECT_EQ(MI4Res->first.getReg(), RISCV::X2);
+  expectDIEPrintResult(
+      MI4Res->second,
+      "!DIExpression(DW_OP_constu, 128, DW_OP_minus, DW_OP_deref_size, 1)");
+
+  MF->deleteMachineBasicBlock(MBB);
+}
+
 } // namespace
 
 INSTANTIATE_TEST_SUITE_P(RV32And64, RISCVInstrInfoTest,

@asb
Copy link
Contributor Author

asb commented Jan 2, 2024

Ping.

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@asb asb merged commit 0b3d1a0 into llvm:main Jan 2, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants