[PATCH] D23931: [XRay] ARM 32-bit no-Thumb support in LLVM

[Dean Michael Berris via llvm-commits]

dberris requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: include/llvm/CodeGen/AsmPrinter.h:209
@@ +208,3 @@
+    bool AlwaysInstrument;
+    const class Function *Fn;
+  };
----------------
Do you need to spell out 'class' here? Wouldn't `const Function*` suffice?

================
Comment at: include/llvm/Target/Target.td:969
@@ +968,3 @@
+  let usesCustomInserter = 1;
+  let hasSideEffects = 0; // FIXME: is this correct?
+  let isReturn = 0; // Original return instruction will follow
----------------
AFAICT, yes, this is correct. The expectation is that this instruction should only ever show up in the assembler as a pseudo instruction (unless this is doing something else).

================
Comment at: include/llvm/Target/Target.td:970
@@ +969,3 @@
+  let hasSideEffects = 0; // FIXME: is this correct?
+  let isReturn = 0; // Original return instruction will follow
+}
----------------
This one is a little harder. At least in x86, we weren't able to get this to work this way, because stack adjustments may happen later than the insertion of the marker instruction. Unless you can control exactly when this instruction is inserted and that the stack adjustment code doesn't ever move this (or add things after this instruction) then you might want to go do the same thing that we're doing in X86.

================
Comment at: include/llvm/Target/TargetOpcodes.def:161
@@ +160,3 @@
+/// for inserting instrumentation instructions at runtime.
+/// The patch here prepends the return instruction.
+HANDLE_TARGET_OPCODE(PATCHABLE_FUNCTION_EXIT)
----------------
Is there any reason to do this instead of following the same convention used in x86 of having the nops be after the return instruction?

================
Comment at: lib/CodeGen/XRayInstrumentation.cpp:46-53
@@ +45,10 @@
+  void replaceRetWithPatchableRet(MachineFunction &MF, const TargetInstrInfo *TII);
+  // Prepend the original return instruction with the exit sled code ("patchable
+  //   function exit" pseudo-instruction), preserving the original return instruction
+  //   just after the exit sled code.
+  // This is the approach to go on CPUs which have multiple options for the return
+  //   instruction, like ARM. For such CPUs we can't just jump into the XRay trampoline
+  //   and issue a single return instruction there. We rather have to call the
+  //   trampoline and return from it to the original return instruction of the
+  //   function being instrumented.
+  void prependRetWithPatchableExit(MachineFunction &MF, const TargetInstrInfo *TII);
----------------
This is a great explanation. Can you say something similar in the description just so it's clear why there's a difference in the approach?

================
Comment at: lib/Target/X86/X86AsmPrinter.h:74-94
@@ -73,23 +73,2 @@
 
-  // This describes the kind of sled we're storing in the XRay table.
-  enum class SledKind : uint8_t {
-    FUNCTION_ENTER = 0,
-    FUNCTION_EXIT = 1,
-    TAIL_CALL = 2,
-  };
-
-  // The table will contain these structs that point to the sled, the function
-  // containing the sled, and what kind of sled (and whether they should always
-  // be instrumented).
-  struct XRayFunctionEntry {
-    const MCSymbol *Sled;
-    const MCSymbol *Function;
-    SledKind Kind;
-    bool AlwaysInstrument;
-    const class Function *Fn;
-  };
-
-  // All the sleds to be emitted.
-  std::vector<XRayFunctionEntry> Sleds;
-
   // All instructions emitted by the X86AsmPrinter should use this helper
----------------
I think it's worth noting in the description that we're moving the XRay instrumentation support up to AsmPrinter too.


https://reviews.llvm.org/D23931