[BOLT] Fix X86MCPlusBuilder::replaceRegWithImm

[Amir]

Summary

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)

[Amir]

This comment was removed by Amir.

[Amir]

Rebase

[Amir]

Fix tail-duplication-prop-bug.s

[rafauler]

Thanks

bolt/lib/Target/X86/X86MCPlusBuilder.cpp

for (unsigned Index = InstDesc.getNumDefs() + (I.HasLHS ? 1 : 0),
          Index != E; ++Index)
       if (Inst.getOperand(Index).isReg() &&
           Inst.getOperand(Index).getReg() == Register)
         NumFound++;
 
     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;
   }
 
   bool replaceRegWithReg(MCInst &Inst, unsigned ToReplace,
                          unsigned ReplaceWith) const override {
 
     // Get the HasLHS value so that iteration can be done

bolt/test/X86/tail-duplication-prop-bug.s

 # 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
 # RUN: link_fdata %s %t.o %t.fdata
 # RUN: llvm-strip --strip-unneeded %t.o
-# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q -nostdlib
+# RUN: ld.lld %t.o -o %t.exe -q -nostdlib
 # RUN: llvm-bolt %t.exe -o %t.out -data %t.fdata -relocs \
 # RUN:   -tail-duplication=1 -tail-duplication-aggressive=1 \
 # RUN:   -tail-duplication-const-copy-propagation=1
 
+  .text
+  .type a, %function
   .globl a
 a:
   .cfi_startproc
   jmpq  *JT(,%rcx,8)
 b:
   jb  d
 # FDATA: 1 a #b# 1 a #d# 6 60
 e:
   cmpl  %eax, %ebx
 f:
   jmp g
 # FDATA: 1 a #f# 1 a #g# 0 8
 d:
   movl  $0x1, %ebx
   jmp e
   jmp g
 h:
   jmp h
 i:
   jne j
 g:
   jmp g
 j:
+    ud2
   .cfi_endproc
+.size a, .-a
+
 .rodata
 JT:
   .quad b
   .quad i

bolt/unittests/Core/MCPlusBuilder.cpp

 #ifdef AARCH64_AVAILABLE
 #include "AArch64Subtarget.h"
 #endif // AARCH64_AVAILABLE
 
 #ifdef X86_AVAILABLE
 #include "X86Subtarget.h"
 #endif // X86_AVAILABLE
 
+#include "bolt/Core/BinaryBasicBlock.h"
+#include "bolt/Core/BinaryFunction.h"
 #include "bolt/Rewrite/RewriteInstance.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Support/TargetSelect.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;

 }
 
 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);
 }
 
+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
 
 TEST_P(MCPlusBuilderTester, Annotation) {
   MCInst Inst;
   bool Success = BC->MIB->createTailCall(Inst, BC->Ctx->createNamedTempSymbol(),
                                          BC->Ctx.get());
   ASSERT_TRUE(Success);
   MCSymbol *LPSymbol = BC->Ctx->createNamedTempSymbol("LP");