[BOLT] Do not return Def-ed registers from MCPlusBuilder::getUsedRegs #129890
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
atrosinenko
requested review from
aaupov,
ayermolo,
dcci,
rafaelauler and
yota9
as code owners
Member
|
@llvm/pr-subscribers-bolt Author: Anatoly Trosinenko (atrosinenko) ChangesUpdate the implementation of Full diff: https://github.com/llvm/llvm-project/pull/129890.diff 2 Files Affected:
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) {
|
Contributor
Author
|
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 /// 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 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
force-pushed
the
users/atrosinenko/bolt-fix-getusedregs
branch
from
83334f0 to
62178df
Compare
Contributor
Author
|
Ping. Rebased the PR onto current
|
maksfb
approved these changes
Apr 22, 2025
Contributor
maksfb
left a comment
maksfb
left a comment
There was a problem hiding this comment.
Thanks! Makes sense to me.
cc: @rafaelauler - the original author of dataflow analysis.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…llvm#129890) Update the implementation of `MCPlusBuilder::getUsedRegs` to match its description in the header file, add unit tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update the implementation of
MCPlusBuilder::getUsedRegsto match its description in the header file, add unit tests.