[XRay][CodeGen][PowerPC] Fix tail exit codegen for XRay in PPC

[dberris]

Summary

This fixes code-gen for XRay in PPC. The regression wasn't caught by
codegen tests which we add in this change.

What happened was the following:

• For tail exits, we used to unconditionally prepend the returns/exits with a pseudo-instruction that gets lowered to the instrumentation sled (and leave the actual return/exit instruction as-is).
• Changes to the XRay instrumentation pass caused the tail exits to suddenly also emit the tail exit pseudo-instruction, since the check for whether a return instruction was also a call instruction meant it was a tail exit instruction.
• None of the tests caught the regression either due to non-existent tests, or the tests being disabled/removed for continuous breakage.

This change re-introduces some of the basic tests and verifies that
we're back to a state that allows the back-end to generate appropriate
XRay instrumented binaries for PPC in the presence of tail exits.

[timshen]

lib/CodeGen/XRayInstrumentation.cpp

118 ↗

(On Diff #114177)

This is actively removing PATCHABLE_TAIL_CALL emits, and it affects all architectures.

Can we add x86 tests for tracking the decisions we've made?

[dberris]

lib/CodeGen/XRayInstrumentation.cpp

118 ↗

(On Diff #114177)

Note that this function only gets called for non-X86 architectures (where prepending is the correct solution).

We already have tests in X86 for the tail call exit sled lowering. Those pass with this change.

[timshen]

lib/CodeGen/XRayInstrumentation.cpp

118 ↗

(On Diff #114177)

We have other non-x86 targets like aarch64, right? How do those targets test this?

[dberris]

lib/CodeGen/XRayInstrumentation.cpp

118 ↗

(On Diff #114177)

Yes, we have the following tests aside from the ones we're adding here.

test/CodeGen/ARM/xray-tail-call-sled.ll
test/CodeGen/AArch64/xray-tail-call-sled.ll

Or did you mean we need a test for tail calls out to hidden/external functions?

[timshen]

lib/CodeGen/XRayInstrumentation.cpp

118 ↗

(On Diff #114177)

What does "out to" mean?

Yes. I was surprised that aarch64 tests are not affected by this patch. I guess that tests are missing and we should add them, but my guess can be wrong?

[dberris]

lib/CodeGen/XRayInstrumentation.cpp

118 ↗

(On Diff #114177)

What does "out to" mean?

I meant, since the callee is only declared and not defined in the same TU.

I suspect the aarch64 tests weren't affected, because TII->isTailCall(T) might not be true for the machine instruction that's used to represent the tail call.

I'm happy to add the tests later if that matters for aarch64, just so we can ensure that if anything changes in this codepath that might affect that target, that we can catch breakages of this form too.

[timshen]

lib/CodeGen/XRayInstrumentation.cpp

118 ↗

(On Diff #114177)

I'm happy to add the tests later if that matters for aarch64, just so we can ensure that if anything changes in this codepath that might affect that target, that we can catch breakages of this form too.

SGTM.

llvm/trunk/lib/CodeGen/XRayInstrumentation.cpp

 //===- XRayInstrumentation.cpp - Adds XRay instrumentation to functions. --===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements a MachineFunctionPass that inserts the appropriate
 // XRay instrumentation instructions. We look for XRay-specific attributes
 // on the function to determine whether we should insert the replacement
 // operations.
 //
 //===---------------------------------------------------------------------===//
 
-#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineDominators.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineLoopInfo.h"
 #include "llvm/IR/Attributes.h"
void XRayInstrumentation::replaceRetWithPatchableRet(
   }
 
   for (auto &I : Terminators)
     I->eraseFromParent();
 }
 
 void XRayInstrumentation::prependRetWithPatchableExit(
     MachineFunction &MF, const TargetInstrInfo *TII) {
-  for (auto &MBB : MF) {
-    for (auto &T : MBB.terminators()) {
-      unsigned Opc = 0;
+  for (auto &MBB : MF)
+    for (auto &T : MBB.terminators())
       if (T.isReturn()) {
-        Opc = TargetOpcode::PATCHABLE_FUNCTION_EXIT;
-      }
-      if (TII->isTailCall(T)) {
-        Opc = TargetOpcode::PATCHABLE_TAIL_CALL;
-      }
-      if (Opc != 0) {
-        // Prepend the return instruction with PATCHABLE_FUNCTION_EXIT or
-        //   PATCHABLE_TAIL_CALL .
-        BuildMI(MBB, T, T.getDebugLoc(), TII->get(Opc));
-      }
-    }
+        // Prepend the return instruction with PATCHABLE_FUNCTION_EXIT.
+        BuildMI(MBB, T, T.getDebugLoc(),
+                TII->get(TargetOpcode::PATCHABLE_FUNCTION_EXIT));
       }
 }
 
 bool XRayInstrumentation::runOnMachineFunction(MachineFunction &MF) {
   auto &F = *MF.getFunction();
   auto InstrAttr = F.getFnAttribute("function-instrument");
   bool AlwaysInstrument = !InstrAttr.hasAttribute(Attribute::None) &&
                           InstrAttr.isStringAttribute() &&
                           InstrAttr.getValueAsString() == "xray-always";
   Attribute Attr = F.getFnAttribute("xray-instruction-threshold");
   unsigned XRayThreshold = 0;
   if (!AlwaysInstrument) {
     if (Attr.hasAttribute(Attribute::None) || !Attr.isStringAttribute())
       return false; // XRay threshold attribute not found.
     if (Attr.getValueAsString().getAsInteger(10, XRayThreshold))
       return false; // Invalid value for threshold.
 
     // Count the number of MachineInstr`s in MachineFunction
     int64_t MICount = 0;
-    for (const auto& MBB : MF)
+    for (const auto &MBB : MF)
       MICount += MBB.size();
 
     // Check if we have a loop.
     // FIXME: Maybe make this smarter, and see whether the loops are dependent
     // on inputs or side-effects?
     MachineLoopInfo &MLI = getAnalysis<MachineLoopInfo>();
     if (MLI.empty() && MICount < XRayThreshold)
       return false; // Function is too small and has no loops.

llvm/trunk/lib/Target/PowerPC/PPCAsmPrinter.cpp

EmitToStreamer(*OutStreamer,
                        .addExpr(MCSymbolRefExpr::create(
                            OutContext.getOrCreateSymbol("__xray_FunctionExit"),
                            OutContext)));
     EmitToStreamer(*OutStreamer, MCInstBuilder(PPC::MTLR8).addReg(PPC::X0));
     recordSled(BeginOfSled, *MI, SledKind::FUNCTION_EXIT);
     break;
   }
   case TargetOpcode::PATCHABLE_TAIL_CALL:
+    // TODO: Define a trampoline `__xray_FunctionTailExit` and differentiate a
+    // normal function exit from a tail exit.
   case TargetOpcode::PATCHABLE_RET:
     // PPC's tail call instruction, e.g. PPC::TCRETURNdi8, doesn't really
     // lower to a PPC::B instruction. The PPC::B instruction is generated
     // before it, and handled by the normal case.
     llvm_unreachable("Tail call is handled in the normal case. See comments"
                      "around this assert.");
   }
 }

llvm/trunk/test/CodeGen/PowerPC/xray-attribute-instrumentation.ll

+; RUN: llc -filetype=asm -o - -mtriple=powerpc64le-unknown-linux-gnu < %s | FileCheck %s
+; RUN: llc -filetype=asm -o - -mtriple=powerpc64le-unknown-linux-gnu \
+; RUN:    -relocation-model=pic < %s | FileCheck %s
+
+define i32 @foo() nounwind noinline uwtable "function-instrument"="xray-always" {
+; CHECK-LABEL: .Ltmp0:
+; CHECK:              b .Ltmp1
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         std 0, -8(1)
+; CHECK-NEXT:         mflr 0
+; CHECK-NEXT:         bl __xray_FunctionEntry
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         mtlr 0
+; CHECK-LABEL: .Ltmp1:
+  ret i32 0
+; CHECK-LABEL: .Ltmp2:
+; CHECK:              blr
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         std 0, -8(1)
+; CHECK-NEXT:         mflr 0
+; CHECK-NEXT:         bl __xray_FunctionExit
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         mtlr 0
+}
+; CHECK-LABEL: xray_instr_map,"awo",@progbits,.text,unique,1
+; CHECK:      .Lxray_sleds_start0:
+; CHECK-NEXT:         .quad   .Ltmp0
+; CHECK-NEXT:         .quad   foo
+; CHECK-NEXT:         .byte   0x00
+; CHECK-NEXT:         .byte   0x01
+; CHECK-NEXT:         .byte   0x00
+; CHECK-NEXT:         .space  13
+; CHECK-NEXT:         .quad   .Ltmp2
+; CHECK-NEXT:         .quad   foo
+; CHECK-NEXT:         .byte   0x01
+; CHECK-NEXT:         .byte   0x01
+; CHECK-NEXT:         .byte   0x00
+; CHECK-NEXT:         .space  13
+; CHECK-NEXT: .Lxray_sleds_end0:
+; CHECK-LABEL: xray_fn_idx,"awo",@progbits,.text,unique,1
+; CHECK:              .p2align        4
+; CHECK-NEXT:         .quad   .Lxray_sleds_start0
+; CHECK-NEXT:         .quad   .Lxray_sleds_end0
+; CHECK-NEXT:         .text

llvm/trunk/test/CodeGen/PowerPC/xray-tail-call-hidden.ll

+; RUN: llc -filetype=asm -o - -mtriple=powerpc64le-unknown-linux-gnu < %s | FileCheck %s
+
+declare hidden i32 @callee() nounwind noinline uwtable "function-instrument"="xray-always"
+
+define i32 @caller() nounwind noinline uwtable "function-instrument"="xray-always" {
+; CHECK-LABEL: .Ltmp0:
+; CHECK:              b .Ltmp1
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         std 0, -8(1)
+; CHECK-NEXT:         mflr 0
+; CHECK-NEXT:         bl __xray_FunctionEntry
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         mtlr 0
+; CHECK-LABEL: .Ltmp1:
+  %retval = tail call i32 @callee()
+  ret i32 %retval
+; CHECK-LABEL: .Ltmp2:
+; CHECK:              b callee
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         std 0, -8(1)
+; CHECK-NEXT:         mflr 0
+; CHECK-NEXT:         bl __xray_FunctionExit
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         mtlr 0
+}
+

llvm/trunk/test/CodeGen/PowerPC/xray-tail-call-sled.ll

+; RUN: llc -filetype=asm -o - -mtriple=powerpc64le-unknown-linux-gnu < %s | FileCheck %s
+
+define i32 @callee() nounwind noinline uwtable "function-instrument"="xray-always" {
+; CHECK-LABEL: .Ltmp0:
+; CHECK:              b .Ltmp1
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         std 0, -8(1)
+; CHECK-NEXT:         mflr 0
+; CHECK-NEXT:         bl __xray_FunctionEntry
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         mtlr 0
+; CHECK-LABEL: .Ltmp1:
+  ret i32 0
+; CHECK-LABEL: .Ltmp2:
+; CHECK:              blr
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         std 0, -8(1)
+; CHECK-NEXT:         mflr 0
+; CHECK-NEXT:         bl __xray_FunctionExit
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         mtlr 0
+}
+
+define i32 @caller() nounwind noinline uwtable "function-instrument"="xray-always" {
+; CHECK-LABEL: .Ltmp3:
+; CHECK:              b .Ltmp4
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         std 0, -8(1)
+; CHECK-NEXT:         mflr 0
+; CHECK-NEXT:         bl __xray_FunctionEntry
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         mtlr 0
+; CHECK-LABEL: .Ltmp4:
+  %retval = tail call i32 @callee()
+  ret i32 %retval
+; CHECK-LABEL: .Ltmp5:
+; CHECK:              blr
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         std 0, -8(1)
+; CHECK-NEXT:         mflr 0
+; CHECK-NEXT:         bl __xray_FunctionExit
+; CHECK-NEXT:         nop
+; CHECK-NEXT:         mtlr 0
+}