[BOLT] Handle generation of compare and jump sequences #131949
Merged
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
rcorcs
requested review from
aaupov,
ayermolo,
dcci,
maksfb,
rafaelauler and
yota9
as code owners
Member
|
@llvm/pr-subscribers-bolt Author: Rodrigo Rocha (rcorcs) ChangesThis patch fixes the following two issues with the createCmpJE for AArch64:
This patch also ds a new function for createCmpJNE and unit tests for the both createCmpJE and createCmpJNE for X86 and AArch64. Full diff: https://github.com/llvm/llvm-project/pull/131949.diff 4 Files Affected:
diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index 1d45a314a17b6..a9d79112b641a 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -1714,6 +1714,16 @@ class MCPlusBuilder {
return {};
}
+ /// Create a sequence of instructions to compare contents of a register
+ /// \p RegNo to immediate \Imm and jump to \p Target if they are different.
+ virtual InstructionListType createCmpJNE(MCPhysReg RegNo, int64_t Imm,
+ const MCSymbol *Target,
+ MCContext *Ctx) const {
+ llvm_unreachable("not implemented");
+ return {};
+ }
+
+
/// Creates inline memcpy instruction. If \p ReturnEnd is true, then return
/// (dest + n) instead of dest.
virtual InstructionListType createInlineMemcpy(bool ReturnEnd) const {
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index 685b2279e5afb..bb627ce2586b0 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -1321,17 +1321,49 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
int getUncondBranchEncodingSize() const override { return 28; }
+ //This helper function creates the snippet of code
+ //that compares a register RegNo with an immedaite Imm,
+ //and jumps to Target if they are equal.
+ //cmp RegNo, #Imm
+ //b.eq Target
+ //where cmp is an for subs, which results in the code below:
+ //subs xzr, RegNo, #Imm
+ //b.eq Target.
InstructionListType createCmpJE(MCPhysReg RegNo, int64_t Imm,
const MCSymbol *Target,
MCContext *Ctx) const override {
InstructionListType Code;
Code.emplace_back(MCInstBuilder(AArch64::SUBSXri)
- .addReg(RegNo)
+ .addReg(AArch64::XZR)
.addReg(RegNo)
.addImm(Imm)
.addImm(0));
Code.emplace_back(MCInstBuilder(AArch64::Bcc)
+ .addImm(AArch64CC::EQ)
+ .addExpr(MCSymbolRefExpr::create(
+ Target, MCSymbolRefExpr::VK_None, *Ctx)));
+ return Code;
+ }
+
+ //This helper function creates the snippet of code
+ //that compares a register RegNo with an immedaite Imm,
+ //and jumps to Target if they are not equal.
+ //cmp RegNo, #Imm
+ //b.ne Target
+ //where cmp is an for subs, which results in the code below:
+ //subs xzr, RegNo, #Imm
+ //b.ne Target.
+ InstructionListType createCmpJNE(MCPhysReg RegNo, int64_t Imm,
+ const MCSymbol *Target,
+ MCContext *Ctx) const override {
+ InstructionListType Code;
+ Code.emplace_back(MCInstBuilder(AArch64::SUBSXri)
+ .addReg(AArch64::XZR)
+ .addReg(RegNo)
.addImm(Imm)
+ .addImm(0));
+ Code.emplace_back(MCInstBuilder(AArch64::Bcc)
+ .addImm(AArch64CC::NE)
.addExpr(MCSymbolRefExpr::create(
Target, MCSymbolRefExpr::VK_None, *Ctx)));
return Code;
diff --git a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
index 465533ee71f2b..bc827e6fd6f1e 100644
--- a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
+++ b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
@@ -2436,6 +2436,20 @@ class X86MCPlusBuilder : public MCPlusBuilder {
return Code;
}
+ InstructionListType createCmpJNE(MCPhysReg RegNo, int64_t Imm,
+ const MCSymbol *Target,
+ MCContext *Ctx) const override {
+ InstructionListType Code;
+ Code.emplace_back(MCInstBuilder(X86::CMP64ri8)
+ .addReg(RegNo)
+ .addImm(Imm));
+ Code.emplace_back(MCInstBuilder(X86::JCC_1)
+ .addExpr(MCSymbolRefExpr::create(
+ Target, MCSymbolRefExpr::VK_None, *Ctx))
+ .addImm(X86::COND_NE));
+ return Code;
+ }
+
std::optional<Relocation>
createRelocation(const MCFixup &Fixup,
const MCAsmBackend &MAB) const override {
diff --git a/bolt/unittests/Core/MCPlusBuilder.cpp b/bolt/unittests/Core/MCPlusBuilder.cpp
index d367eb07f7767..5cd9ee2a6ff76 100644
--- a/bolt/unittests/Core/MCPlusBuilder.cpp
+++ b/bolt/unittests/Core/MCPlusBuilder.cpp
@@ -107,6 +107,52 @@ TEST_P(MCPlusBuilderTester, AliasSmallerX0) {
testRegAliases(Triple::aarch64, AArch64::X0, AliasesX0, AliasesX0Count, true);
}
+TEST_P(MCPlusBuilderTester, AArch64_CmpJE) {
+ if (GetParam() != Triple::aarch64)
+ GTEST_SKIP();
+ BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
+ std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
+
+ InstructionListType Instrs = BC->MIB->createCmpJE(AArch64::X0, 2, BB->getLabel(), BC->Ctx.get());
+ BB->addInstructions(Instrs.begin(), Instrs.end());
+ BB->addSuccessor(BB.get());
+
+ auto II = BB->begin();
+ ASSERT_EQ(II->getOpcode(), AArch64::SUBSXri);
+ ASSERT_EQ(II->getOperand(0).getReg(), AArch64::XZR);
+ ASSERT_EQ(II->getOperand(1).getReg(), AArch64::X0);
+ ASSERT_EQ(II->getOperand(2).getImm(), 2);
+ ASSERT_EQ(II->getOperand(3).getImm(), 0);
+ II++;
+ ASSERT_EQ(II->getOpcode(), AArch64::Bcc);
+ ASSERT_EQ(II->getOperand(0).getImm(), AArch64CC::EQ);
+ const MCSymbol *Label = BC->MIB->getTargetSymbol(*II,1);
+ ASSERT_EQ(Label, BB->getLabel());
+}
+
+TEST_P(MCPlusBuilderTester, AArch64_CmpJNE) {
+ if (GetParam() != Triple::aarch64)
+ GTEST_SKIP();
+ BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
+ std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
+
+ InstructionListType Instrs = BC->MIB->createCmpJNE(AArch64::X0, 2, BB->getLabel(), BC->Ctx.get());
+ BB->addInstructions(Instrs.begin(), Instrs.end());
+ BB->addSuccessor(BB.get());
+
+ auto II = BB->begin();
+ ASSERT_EQ(II->getOpcode(), AArch64::SUBSXri);
+ ASSERT_EQ(II->getOperand(0).getReg(), AArch64::XZR);
+ ASSERT_EQ(II->getOperand(1).getReg(), AArch64::X0);
+ ASSERT_EQ(II->getOperand(2).getImm(), 2);
+ ASSERT_EQ(II->getOperand(3).getImm(), 0);
+ II++;
+ ASSERT_EQ(II->getOpcode(), AArch64::Bcc);
+ ASSERT_EQ(II->getOperand(0).getImm(), AArch64CC::NE);
+ const MCSymbol *Label = BC->MIB->getTargetSymbol(*II,1);
+ ASSERT_EQ(Label, BB->getLabel());
+}
+
#endif // AARCH64_AVAILABLE
#ifdef X86_AVAILABLE
@@ -143,6 +189,48 @@ TEST_P(MCPlusBuilderTester, ReplaceRegWithImm) {
ASSERT_EQ(II->getOperand(1).getImm(), 1);
}
+TEST_P(MCPlusBuilderTester, X86_CmpJE) {
+ if (GetParam() != Triple::x86_64)
+ GTEST_SKIP();
+ BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
+ std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
+
+ InstructionListType Instrs = BC->MIB->createCmpJE(X86::EAX, 2, BB->getLabel(), BC->Ctx.get());
+ BB->addInstructions(Instrs.begin(), Instrs.end());
+ BB->addSuccessor(BB.get());
+
+ auto II = BB->begin();
+ ASSERT_EQ(II->getOpcode(), X86::CMP64ri8);
+ ASSERT_EQ(II->getOperand(0).getReg(), X86::EAX);
+ ASSERT_EQ(II->getOperand(1).getImm(), 2);
+ II++;
+ ASSERT_EQ(II->getOpcode(), X86::JCC_1);
+ const MCSymbol *Label = BC->MIB->getTargetSymbol(*II,0);
+ ASSERT_EQ(Label, BB->getLabel());
+ ASSERT_EQ(II->getOperand(1).getImm(), X86::COND_E);
+}
+
+TEST_P(MCPlusBuilderTester, X86_CmpJNE) {
+ if (GetParam() != Triple::x86_64)
+ GTEST_SKIP();
+ BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
+ std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
+
+ InstructionListType Instrs = BC->MIB->createCmpJNE(X86::EAX, 2, BB->getLabel(), BC->Ctx.get());
+ BB->addInstructions(Instrs.begin(), Instrs.end());
+ BB->addSuccessor(BB.get());
+
+ auto II = BB->begin();
+ ASSERT_EQ(II->getOpcode(), X86::CMP64ri8);
+ ASSERT_EQ(II->getOperand(0).getReg(), X86::EAX);
+ ASSERT_EQ(II->getOperand(1).getImm(), 2);
+ II++;
+ ASSERT_EQ(II->getOpcode(), X86::JCC_1);
+ const MCSymbol *Label = BC->MIB->getTargetSymbol(*II,0);
+ ASSERT_EQ(Label, BB->getLabel());
+ ASSERT_EQ(II->getOperand(1).getImm(), X86::COND_NE);
+}
+
#endif // X86_AVAILABLE
TEST_P(MCPlusBuilderTester, Annotation) {
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
maksfb
reviewed
Mar 19, 2025
Contributor
maksfb
left a comment
maksfb
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
The change to createCmpJE() modifies the code generated by createInstrumentedIndCallHandlerEntryBB(). Was the code broken before? Particularly, what was the result of passing Imm to Bcc? We can use instrumentation as a test case for this change in addition to the unit test.
What is the reason for introducing createCmpJNE()?
Contributor
Author
|
Hi @maksfb , thank you for your comments.
Below is a code snippet from Below is a code snippet from However, note in the example below, that the While the immediate value passed to
The use case for this is a work in progress. |
maksfb
approved these changes
Apr 2, 2025
Contributor
maksfb
left a comment
maksfb
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for detailed explanation. Please format the title to align with LLVM guidelines before committing. Namely, change to imperative form, drop trailing period, and limit the length.
rcorcs
changed the title
Contributor
Author
|
@maksfb thanks for approving this PR. I don't have commit/merge permission. Could you please help me merge this PR? |
maksfb
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This patch fixes the following two issues with the createCmpJE for AArch64:
subs xzr, RegNo, #Imm
which is equivalent to a simple
cmp RegNo, #Imm
This patch also adds a new function for createCmpJNE and unit tests for the both createCmpJE and createCmpJNE for X86 and AArch64.