[BOLT] Do not return Def-ed registers from MCPlusBuilder::getUsedRegs #129890

atrosinenko · 2025-03-05T15:24:46Z

Update the implementation of MCPlusBuilder::getUsedRegs to match its description in the header file, add unit tests.

llvmbot · 2025-03-05T15:34:29Z

@llvm/pr-subscribers-bolt

Author: Anatoly Trosinenko (atrosinenko)

Changes

Update the implementation of MCPlusBuilder::getUsedRegs to match its description in the header file, add unit tests.

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

2 Files Affected:

(modified) bolt/lib/Core/MCPlusBuilder.cpp (+3-3)
(modified) bolt/unittests/Core/MCPlusBuilder.cpp (+122-18)

diff --git a/bolt/lib/Core/MCPlusBuilder.cpp b/bolt/lib/Core/MCPlusBuilder.cpp
index 7ff7a2288451c..20e786afc6ccf 100644
--- a/bolt/lib/Core/MCPlusBuilder.cpp
+++ b/bolt/lib/Core/MCPlusBuilder.cpp
@@ -442,10 +442,10 @@ void MCPlusBuilder::getUsedRegs(const MCInst &Inst, BitVector &Regs) const {
   for (MCPhysReg ImplicitUse : InstInfo.implicit_uses())
     Regs |= getAliases(ImplicitUse, /*OnlySmaller=*/true);
 
-  for (unsigned I = 0, E = Inst.getNumOperands(); I != E; ++I) {
-    if (!Inst.getOperand(I).isReg())
+  for (const MCOperand &Operand : useOperands(Inst)) {
+    if (!Operand.isReg())
       continue;
-    Regs |= getAliases(Inst.getOperand(I).getReg(), /*OnlySmaller=*/true);
+    Regs |= getAliases(Operand.getReg(), /*OnlySmaller=*/true);
   }
 }
 
diff --git a/bolt/unittests/Core/MCPlusBuilder.cpp b/bolt/unittests/Core/MCPlusBuilder.cpp
index d367eb07f7767..df5bb0a3dfbd6 100644
--- a/bolt/unittests/Core/MCPlusBuilder.cpp
+++ b/bolt/unittests/Core/MCPlusBuilder.cpp
@@ -8,6 +8,7 @@
 
 #ifdef AARCH64_AVAILABLE
 #include "AArch64Subtarget.h"
+#include "MCTargetDesc/AArch64MCTargetDesc.h"
 #endif // AARCH64_AVAILABLE
 
 #ifdef X86_AVAILABLE
@@ -19,6 +20,7 @@
 #include "bolt/Rewrite/RewriteInstance.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
+#include "llvm/MC/MCInstBuilder.h"
 #include "llvm/Support/TargetSelect.h"
 #include "gtest/gtest.h"
 
@@ -70,16 +72,28 @@ struct MCPlusBuilderTester : public testing::TestWithParam<Triple::ArchType> {
                             BC->MRI.get(), BC->STI.get())));
   }
 
+  void assertRegMask(const BitVector &RegMask,
+                     std::initializer_list<MCPhysReg> ExpectedRegs) {
+    ASSERT_EQ(RegMask.count(), ExpectedRegs.size());
+    for (MCPhysReg Reg : ExpectedRegs)
+      ASSERT_TRUE(RegMask[Reg]) << "Expected " << BC->MRI->getName(Reg) << ".";
+  }
+
+  void assertRegMask(std::function<void(BitVector &)> FillRegMask,
+                     std::initializer_list<MCPhysReg> ExpectedRegs) {
+    BitVector RegMask(BC->MRI->getNumRegs());
+    FillRegMask(RegMask);
+    assertRegMask(RegMask, ExpectedRegs);
+  }
+
   void testRegAliases(Triple::ArchType Arch, uint64_t Register,
-                      uint64_t *Aliases, size_t Count,
+                      std::initializer_list<MCPhysReg> ExpectedAliases,
                       bool OnlySmaller = false) {
     if (GetParam() != Arch)
       GTEST_SKIP();
 
     const BitVector &BV = BC->MIB->getAliases(Register, OnlySmaller);
-    ASSERT_EQ(BV.count(), Count);
-    for (size_t I = 0; I < Count; ++I)
-      ASSERT_TRUE(BV[Aliases[I]]);
+    assertRegMask(BV, ExpectedAliases);
   }
 
   char ElfBuf[sizeof(typename ELF64LE::Ehdr)] = {};
@@ -94,17 +108,109 @@ INSTANTIATE_TEST_SUITE_P(AArch64, MCPlusBuilderTester,
                          ::testing::Values(Triple::aarch64));
 
 TEST_P(MCPlusBuilderTester, AliasX0) {
-  uint64_t AliasesX0[] = {AArch64::W0,    AArch64::W0_HI,
-                          AArch64::X0,    AArch64::W0_W1,
-                          AArch64::X0_X1, AArch64::X0_X1_X2_X3_X4_X5_X6_X7};
-  size_t AliasesX0Count = sizeof(AliasesX0) / sizeof(*AliasesX0);
-  testRegAliases(Triple::aarch64, AArch64::X0, AliasesX0, AliasesX0Count);
+  testRegAliases(Triple::aarch64, AArch64::X0,
+                 {AArch64::W0, AArch64::W0_HI, AArch64::X0, AArch64::W0_W1,
+                  AArch64::X0_X1, AArch64::X0_X1_X2_X3_X4_X5_X6_X7});
 }
 
 TEST_P(MCPlusBuilderTester, AliasSmallerX0) {
-  uint64_t AliasesX0[] = {AArch64::W0, AArch64::W0_HI, AArch64::X0};
-  size_t AliasesX0Count = sizeof(AliasesX0) / sizeof(*AliasesX0);
-  testRegAliases(Triple::aarch64, AArch64::X0, AliasesX0, AliasesX0Count, true);
+  testRegAliases(Triple::aarch64, AArch64::X0,
+                 {AArch64::W0, AArch64::W0_HI, AArch64::X0},
+                 /*OnlySmaller=*/true);
+}
+
+TEST_P(MCPlusBuilderTester, testAccessedRegsImplicitDef) {
+  if (GetParam() != Triple::aarch64)
+    GTEST_SKIP();
+
+  // adds x0, x5, #42
+  MCInst Inst = MCInstBuilder(AArch64::ADDSXri)
+                    .addReg(AArch64::X0)
+                    .addReg(AArch64::X5)
+                    .addImm(42)
+                    .addImm(0);
+
+  assertRegMask([&](BitVector &BV) { BC->MIB->getClobberedRegs(Inst, BV); },
+                {AArch64::NZCV, AArch64::W0, AArch64::X0, AArch64::W0_HI,
+                 AArch64::X0_X1_X2_X3_X4_X5_X6_X7, AArch64::W0_W1,
+                 AArch64::X0_X1});
+
+  assertRegMask(
+      [&](BitVector &BV) { BC->MIB->getTouchedRegs(Inst, BV); },
+      {AArch64::NZCV, AArch64::W0, AArch64::W5, AArch64::X0, AArch64::X5,
+       AArch64::W0_HI, AArch64::W5_HI, AArch64::X0_X1_X2_X3_X4_X5_X6_X7,
+       AArch64::X2_X3_X4_X5_X6_X7_X8_X9, AArch64::X4_X5_X6_X7_X8_X9_X10_X11,
+       AArch64::W0_W1, AArch64::W4_W5, AArch64::X0_X1, AArch64::X4_X5});
+
+  assertRegMask([&](BitVector &BV) { BC->MIB->getWrittenRegs(Inst, BV); },
+                {AArch64::NZCV, AArch64::W0, AArch64::X0, AArch64::W0_HI});
+
+  assertRegMask([&](BitVector &BV) { BC->MIB->getUsedRegs(Inst, BV); },
+                {AArch64::W5, AArch64::X5, AArch64::W5_HI});
+
+  assertRegMask([&](BitVector &BV) { BC->MIB->getSrcRegs(Inst, BV); },
+                {AArch64::W5, AArch64::X5, AArch64::W5_HI});
+}
+
+TEST_P(MCPlusBuilderTester, testAccessedRegsImplicitUse) {
+  if (GetParam() != Triple::aarch64)
+    GTEST_SKIP();
+
+  // b.eq <label>
+  MCInst Inst =
+      MCInstBuilder(AArch64::Bcc)
+          .addImm(AArch64CC::EQ)
+          .addImm(0); // <label> - should be Expr, but immediate 0 works too.
+
+  assertRegMask([&](BitVector &BV) { BC->MIB->getClobberedRegs(Inst, BV); },
+                {});
+
+  assertRegMask([&](BitVector &BV) { BC->MIB->getTouchedRegs(Inst, BV); },
+                {AArch64::NZCV});
+
+  assertRegMask([&](BitVector &BV) { BC->MIB->getWrittenRegs(Inst, BV); }, {});
+
+  assertRegMask([&](BitVector &BV) { BC->MIB->getUsedRegs(Inst, BV); },
+                {AArch64::NZCV});
+
+  assertRegMask([&](BitVector &BV) { BC->MIB->getSrcRegs(Inst, BV); },
+                {AArch64::NZCV});
+}
+
+TEST_P(MCPlusBuilderTester, testAccessedRegsMultipleDefs) {
+  if (GetParam() != Triple::aarch64)
+    GTEST_SKIP();
+
+  // ldr x0, [x5], #16
+  MCInst Inst = MCInstBuilder(AArch64::LDRXpost)
+                    .addReg(AArch64::X5)
+                    .addReg(AArch64::X0)
+                    .addReg(AArch64::X5)
+                    .addImm(16);
+
+  assertRegMask(
+      [&](BitVector &BV) { BC->MIB->getClobberedRegs(Inst, BV); },
+      {AArch64::W0, AArch64::W5, AArch64::X0, AArch64::X5, AArch64::W0_HI,
+       AArch64::W5_HI, AArch64::X0_X1_X2_X3_X4_X5_X6_X7,
+       AArch64::X2_X3_X4_X5_X6_X7_X8_X9, AArch64::X4_X5_X6_X7_X8_X9_X10_X11,
+       AArch64::W0_W1, AArch64::W4_W5, AArch64::X0_X1, AArch64::X4_X5});
+
+  assertRegMask(
+      [&](BitVector &BV) { BC->MIB->getTouchedRegs(Inst, BV); },
+      {AArch64::W0, AArch64::W5, AArch64::X0, AArch64::X5, AArch64::W0_HI,
+       AArch64::W5_HI, AArch64::X0_X1_X2_X3_X4_X5_X6_X7,
+       AArch64::X2_X3_X4_X5_X6_X7_X8_X9, AArch64::X4_X5_X6_X7_X8_X9_X10_X11,
+       AArch64::W0_W1, AArch64::W4_W5, AArch64::X0_X1, AArch64::X4_X5});
+
+  assertRegMask([&](BitVector &BV) { BC->MIB->getWrittenRegs(Inst, BV); },
+                {AArch64::W0, AArch64::X0, AArch64::W0_HI, AArch64::W5,
+                 AArch64::X5, AArch64::W5_HI});
+
+  assertRegMask([&](BitVector &BV) { BC->MIB->getUsedRegs(Inst, BV); },
+                {AArch64::W5, AArch64::X5, AArch64::W5_HI});
+
+  assertRegMask([&](BitVector &BV) { BC->MIB->getSrcRegs(Inst, BV); },
+                {AArch64::W5, AArch64::X5, AArch64::W5_HI});
 }
 
 #endif // AARCH64_AVAILABLE
@@ -115,15 +221,13 @@ INSTANTIATE_TEST_SUITE_P(X86, MCPlusBuilderTester,
                          ::testing::Values(Triple::x86_64));
 
 TEST_P(MCPlusBuilderTester, AliasAX) {
-  uint64_t AliasesAX[] = {X86::RAX, X86::EAX, X86::AX, X86::AL, X86::AH};
-  size_t AliasesAXCount = sizeof(AliasesAX) / sizeof(*AliasesAX);
-  testRegAliases(Triple::x86_64, X86::AX, AliasesAX, AliasesAXCount);
+  testRegAliases(Triple::x86_64, X86::AX,
+                 {X86::RAX, X86::EAX, X86::AX, X86::AL, X86::AH});
 }
 
 TEST_P(MCPlusBuilderTester, AliasSmallerAX) {
-  uint64_t AliasesAX[] = {X86::AX, X86::AL, X86::AH};
-  size_t AliasesAXCount = sizeof(AliasesAX) / sizeof(*AliasesAX);
-  testRegAliases(Triple::x86_64, X86::AX, AliasesAX, AliasesAXCount, true);
+  testRegAliases(Triple::x86_64, X86::AX, {X86::AX, X86::AL, X86::AH},
+                 /*OnlySmaller=*/true);
 }
 
 TEST_P(MCPlusBuilderTester, ReplaceRegWithImm) {

atrosinenko · 2025-03-05T15:44:38Z

I wonder if there can be existing code that relies on the old behavior. Though, even if not taking the discrepancy between the documentation and the code into account, old implementation doesn't look consistent to me as it inspects all explicit register operands, but only checks implicit uses.

Furthermore, I only managed to find a single direct user of getUsedRegs in the existing code, the RegAnalysis::getInstUsedRegsList function which is documented as

  /// Compute the set of registers \p Inst may read from, marking them in
  /// \p RegSet. If GetClobbers is true, the set set the instr may write to.
  /// Use the callgraph to fill out this info for calls.
  void getInstUsedRegsList(const MCInst &Inst, BitVector &RegSet,
                           bool GetClobbers) const;

and getUsedRegs is only called when GetClobbers is false:

void RegAnalysis::getInstUsedRegsList(const MCInst &Inst, BitVector &RegSet,
                                      bool GetClobbers) const {
  if (!BC.MIB->isCall(Inst)) {
    if (GetClobbers)
      BC.MIB->getClobberedRegs(Inst, RegSet);
    else
      BC.MIB->getUsedRegs(Inst, RegSet);
    return;
  }
// ...

Update the implementation of MCPlusBuilder::getUsedRegs to match its description in the header file, add unit tests.

atrosinenko · 2025-04-21T16:15:06Z

Ping.

Rebased the PR onto current main branch. To simplify the review, the patch is comprised of two commits:

The first one updates getUsedRegs and adds corresponding unit tests. Along the test cases, it adds two assertRegMask helper functions which generalize the idea of testRegAliases and add a bit of std::initializer_list syntactic sugar.
The second commit makes use of assertRegMask in the original testRegAliases function and updates the rest of the test cases to use the same style of passing their register lists.

maksfb

Thanks! Makes sense to me.

cc: @rafaelauler - the original author of dataflow analysis.

…llvm#129890) Update the implementation of `MCPlusBuilder::getUsedRegs` to match its description in the header file, add unit tests.

atrosinenko requested review from sdesmalen-arm, maksfb and asl March 5, 2025 15:30

atrosinenko marked this pull request as ready for review March 5, 2025 15:33

atrosinenko requested review from aaupov, rafaelauler, ayermolo, dcci and yota9 as code owners March 5, 2025 15:33

llvmbot added the BOLT label Mar 5, 2025

atrosinenko added 2 commits April 21, 2025 17:39

[BOLT] Do not return Def-ed registers from MCPlusBuilder::getUsedRegs

b030705

Update the implementation of MCPlusBuilder::getUsedRegs to match its description in the header file, add unit tests.

Update bolt/unittests/Core/MCPlusBuilder.cpp

62178df

atrosinenko force-pushed the users/atrosinenko/bolt-fix-getusedregs branch from 83334f0 to 62178df Compare April 21, 2025 16:04

maksfb approved these changes Apr 22, 2025

View reviewed changes

atrosinenko merged commit 37e8c6c into main Apr 23, 2025
10 checks passed

atrosinenko deleted the users/atrosinenko/bolt-fix-getusedregs branch April 23, 2025 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[BOLT] Do not return Def-ed registers from MCPlusBuilder::getUsedRegs #129890

[BOLT] Do not return Def-ed registers from MCPlusBuilder::getUsedRegs #129890

Uh oh!

atrosinenko commented Mar 5, 2025

Uh oh!

llvmbot commented Mar 5, 2025

Uh oh!

atrosinenko commented Mar 5, 2025

Uh oh!

atrosinenko commented Apr 21, 2025

Uh oh!

maksfb left a comment

Uh oh!

Uh oh!

Uh oh!

[BOLT] Do not return Def-ed registers from MCPlusBuilder::getUsedRegs #129890

[BOLT] Do not return Def-ed registers from MCPlusBuilder::getUsedRegs #129890

Uh oh!

Conversation

atrosinenko commented Mar 5, 2025

Uh oh!

llvmbot commented Mar 5, 2025

Uh oh!

atrosinenko commented Mar 5, 2025

Uh oh!

atrosinenko commented Apr 21, 2025

Uh oh!

maksfb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!