Skip to content

Commit

Permalink
[X86] Use 64-bit version of source register in LowerPATCHABLE_EVENT_C…
Browse files Browse the repository at this point in the history
…ALL and LowerPATCHABLE_TYPED_EVENT_CALL

Summary:
The PATCHABLE_EVENT_CALL uses i32 in the intrinsic. This
results in the register allocator picking a 32-bit register. We
need to use the 64-bit register when forming the MOV64rr
instructions. Otherwise we print illegal assembly in the text
output.

I think prior to this it was impossible for SrcReg to be equal
to DstReg so the NOP code was not reachable.

While there use Register instead of unsigned.

Also add a FIXME for what looks like a bug.

Reviewers: dberris

Reviewed By: dberris

Subscribers: hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D69365
  • Loading branch information
topperc committed Oct 28, 2019
1 parent 9b0b626 commit 7af8d52
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 12 deletions.
18 changes: 12 additions & 6 deletions llvm/lib/Target/X86/X86MCInstLower.cpp
Expand Up @@ -1337,10 +1337,10 @@ void X86AsmPrinter::LowerPATCHABLE_EVENT_CALL(const MachineInstr &MI,

// The default C calling convention will place two arguments into %rcx and
// %rdx -- so we only work with those.
unsigned DestRegs[] = {X86::RDI, X86::RSI};
const Register DestRegs[] = {X86::RDI, X86::RSI};
bool UsedMask[] = {false, false};
// Filled out in loop.
unsigned SrcRegs[] = {0, 0};
Register SrcRegs[] = {0, 0};

// Then we put the operands in the %rdi and %rsi registers. We spill the
// values in the register before we clobber them, and mark them as used in
Expand All @@ -1350,7 +1350,7 @@ void X86AsmPrinter::LowerPATCHABLE_EVENT_CALL(const MachineInstr &MI,
for (unsigned I = 0; I < MI.getNumOperands(); ++I)
if (auto Op = MCIL.LowerMachineOperand(&MI, MI.getOperand(I))) {
assert(Op->isReg() && "Only support arguments in registers");
SrcRegs[I] = Op->getReg();
SrcRegs[I] = getX86SubSuperRegister(Op->getReg(), 64);
if (SrcRegs[I] != DestRegs[I]) {
UsedMask[I] = true;
EmitAndCountInstruction(
Expand All @@ -1361,6 +1361,9 @@ void X86AsmPrinter::LowerPATCHABLE_EVENT_CALL(const MachineInstr &MI,
}

// Now that the register values are stashed, mov arguments into place.
// FIXME: This doesn't work if one of the later SrcRegs is equal to an
// earlier DestReg. We will have already overwritten over the register before
// we can copy from it.
for (unsigned I = 0; I < MI.getNumOperands(); ++I)
if (SrcRegs[I] != DestRegs[I])
EmitAndCountInstruction(
Expand Down Expand Up @@ -1429,11 +1432,11 @@ void X86AsmPrinter::LowerPATCHABLE_TYPED_EVENT_CALL(const MachineInstr &MI,
// An x86-64 convention may place three arguments into %rcx, %rdx, and R8,
// so we'll work with those. Or we may be called via SystemV, in which case
// we don't have to do any translation.
unsigned DestRegs[] = {X86::RDI, X86::RSI, X86::RDX};
const Register DestRegs[] = {X86::RDI, X86::RSI, X86::RDX};
bool UsedMask[] = {false, false, false};

// Will fill out src regs in the loop.
unsigned SrcRegs[] = {0, 0, 0};
Register SrcRegs[] = {0, 0, 0};

// Then we put the operands in the SystemV registers. We spill the values in
// the registers before we clobber them, and mark them as used in UsedMask.
Expand All @@ -1443,7 +1446,7 @@ void X86AsmPrinter::LowerPATCHABLE_TYPED_EVENT_CALL(const MachineInstr &MI,
if (auto Op = MCIL.LowerMachineOperand(&MI, MI.getOperand(I))) {
// TODO: Is register only support adequate?
assert(Op->isReg() && "Only supports arguments in registers");
SrcRegs[I] = Op->getReg();
SrcRegs[I] = getX86SubSuperRegister(Op->getReg(), 64);
if (SrcRegs[I] != DestRegs[I]) {
UsedMask[I] = true;
EmitAndCountInstruction(
Expand All @@ -1459,6 +1462,9 @@ void X86AsmPrinter::LowerPATCHABLE_TYPED_EVENT_CALL(const MachineInstr &MI,
// is clobbers. We've already added nops to account for the size of mov and
// push if the register is in the right place, so we only have to worry about
// emitting movs.
// FIXME: This doesn't work if one of the later SrcRegs is equal to an
// earlier DestReg. We will have already overwritten over the register before
// we can copy from it.
for (unsigned I = 0; I < MI.getNumOperands(); ++I)
if (UsedMask[I])
EmitAndCountInstruction(
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/CodeGen/X86/xray-custom-log.ll
Expand Up @@ -13,7 +13,7 @@ define i32 @fn() nounwind noinline uwtable "function-instrument"="xray-always" {
; CHECK-NEXT: pushq %rdi
; CHECK-NEXT: pushq %rsi
; CHECK-NEXT: movq %rcx, %rdi
; CHECK-NEXT: movq %eax, %rsi
; CHECK-NEXT: movq %rax, %rsi
; CHECK-NEXT: callq __xray_CustomEvent
; CHECK-NEXT: popq %rsi
; CHECK-NEXT: popq %rdi
Expand All @@ -23,7 +23,7 @@ define i32 @fn() nounwind noinline uwtable "function-instrument"="xray-always" {
; PIC-NEXT: pushq %rdi
; PIC-NEXT: pushq %rsi
; PIC-NEXT: movq %rcx, %rdi
; PIC-NEXT: movq %eax, %rsi
; PIC-NEXT: movq %rax, %rsi
; PIC-NEXT: callq __xray_CustomEvent@PLT
; PIC-NEXT: popq %rsi
; PIC-NEXT: popq %rdi
Expand Down
8 changes: 4 additions & 4 deletions llvm/test/CodeGen/X86/xray-typed-event-log.ll
Expand Up @@ -16,9 +16,9 @@ define i32 @fn() nounwind noinline uwtable "function-instrument"="xray-always" {
; CHECK-NEXT: pushq %rdi
; CHECK-NEXT: pushq %rsi
; CHECK-NEXT: pushq %rdx
; CHECK-NEXT: movq %dx, %rdi
; CHECK-NEXT: movq %rdx, %rdi
; CHECK-NEXT: movq %rcx, %rsi
; CHECK-NEXT: movq %eax, %rdx
; CHECK-NEXT: movq %rax, %rdx
; CHECK-NEXT: callq __xray_TypedEvent
; CHECK-NEXT: popq %rdx
; CHECK-NEXT: popq %rsi
Expand All @@ -29,9 +29,9 @@ define i32 @fn() nounwind noinline uwtable "function-instrument"="xray-always" {
; PIC-NEXT: pushq %rdi
; PIC-NEXT: pushq %rsi
; PIC-NEXT: pushq %rdx
; PIC-NEXT: movq %dx, %rdi
; PIC-NEXT: movq %rdx, %rdi
; PIC-NEXT: movq %rcx, %rsi
; PIC-NEXT: movq %eax, %rdx
; PIC-NEXT: movq %rax, %rdx
; PIC-NEXT: callq __xray_TypedEvent@PLT
; PIC-NEXT: popq %rdx
; PIC-NEXT: popq %rsi
Expand Down

0 comments on commit 7af8d52

Please sign in to comment.