[PATCH] D120263: [BOLT] Fix X86MCPlusBuilder::replaceRegWithImm

[Amir Ayupov via Phabricator via llvm-commits]

Amir created this revision.
Herald added subscribers: ayermolo, pengfei.
Herald added a reviewer: rafauler.
Herald added a reviewer: maksfb.
Amir requested review of this revision.
Herald added subscribers: llvm-commits, yota9.
Herald added a project: LLVM.

Reassigning the operand didn't update the operand type which resulted in an
assertion (`Assertion `isReg() && "This is not a register operand!"' failed.`)
Reset the instruction instead.

Test Plan:

  ninja check-bolt
  ...
  PASS: BOLT-Unit :: Core/./CoreTests/X86/MCPlusBuilderTester.ReplaceRegWithImm/0 (90 of 136)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120263

Files:
  bolt/lib/Target/X86/X86MCPlusBuilder.cpp
  bolt/test/X86/tail-duplication-prop-bug.s
  bolt/unittests/Core/MCPlusBuilder.cpp


Index: bolt/unittests/Core/MCPlusBuilder.cpp
===================================================================
--- bolt/unittests/Core/MCPlusBuilder.cpp
+++ bolt/unittests/Core/MCPlusBuilder.cpp
@@ -6,6 +6,9 @@
 #include "X86Subtarget.h"
 #endif // X86_AVAILABLE
 
+#include "bolt/Core/BinaryBasicBlock.h"
+#include "bolt/Core/BinaryFunction.h"
+#include "bolt/Core/MCPlusAnnotation.h"
 #include "bolt/Rewrite/RewriteInstance.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
@@ -109,4 +112,21 @@
   testRegAliases(Triple::x86_64, X86::AX, AliasesAX, AliasesAXCount, true);
 }
 
+TEST_P(MCPlusBuilderTester, ReplaceRegWithImm) {
+  if (GetParam() != Triple::x86_64)
+    GTEST_SKIP();
+  BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
+  std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock(0);
+  MCInst Inst; // cmpl    %eax, %ebx
+  Inst.setOpcode(X86::CMP32rr);
+  Inst.addOperand(MCOperand::createReg(X86::EAX));
+  Inst.addOperand(MCOperand::createReg(X86::EBX));
+  auto II = BB->addInstruction(Inst);
+  bool Replaced = BC->MIB->replaceRegWithImm(*II, X86::EBX, 1);
+  ASSERT_TRUE(Replaced);
+  ASSERT_EQ(II->getOpcode(), X86::CMP32ri8);
+  ASSERT_EQ(II->getOperand(0).getReg(), X86::EAX);
+  ASSERT_EQ(II->getOperand(1).getImm(), 1);
+}
+
 #endif // X86_AVAILABLE
Index: bolt/test/X86/tail-duplication-prop-bug.s
===================================================================
--- bolt/test/X86/tail-duplication-prop-bug.s
+++ bolt/test/X86/tail-duplication-prop-bug.s
@@ -1,5 +1,4 @@
 # This reproduces a bug in aggressive tail duplication/copy propagation.
-# XFAIL: *
 
 # REQUIRES: system-linux
 # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o
Index: bolt/lib/Target/X86/X86MCPlusBuilder.cpp
===================================================================
--- bolt/lib/Target/X86/X86MCPlusBuilder.cpp
+++ bolt/lib/Target/X86/X86MCPlusBuilder.cpp
@@ -3030,21 +3030,13 @@
     if (NumFound != 1)
       return false;
 
-    // Iterate backwards to replace the src register before the src/dest
-    // register as in AND, ADD, and SUB Only iterate through src operands that
-    // arent also dest operands
-    for (unsigned Index = InstDesc.getNumOperands() - 1,
-                  E = InstDesc.getNumDefs() + (I.HasLHS ? 0 : -1);
-         Index != E; --Index) {
-      if (!Inst.getOperand(Index).isReg() ||
-          Inst.getOperand(Index).getReg() != Register)
-        continue;
-      MCOperand NewOperand = MCOperand::createImm(Imm);
-      Inst.getOperand(Index) = NewOperand;
-      break;
-    }
-
+    MCOperand TargetOp = Inst.getOperand(0);
+    Inst.clear();
     Inst.setOpcode(NewOpcode);
+    Inst.addOperand(TargetOp);
+    if (I.HasLHS)
+      Inst.addOperand(TargetOp);
+    Inst.addOperand(MCOperand::createImm(Imm));
 
     return true;
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D120263.410333.patch
Type: text/x-patch
Size: 2874 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220221/4e82974a/attachment.bin>