[PATCH] D27503: [XRay] Custom event logging intrinsic

[Tim Shen via Phabricator via llvm-commits]

timshen accepted this revision.
timshen added inline comments.


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1052
+  //   .p2align 1, ...
+  // .Lxray_event_sled_N:
+  //   jmp +N                    // jump across the call instruction
----------------
dberris wrote:
> timshen wrote:
> > dberris wrote:
> > > timshen wrote:
> > > > I believe that before at symbol, all caller-saved registers are actually saved?
> > > I thought that too, except the calling convention for the intrinsic seems to be the C calling convention (default) as opposed to the SysV64 calling convention that I'd like it to be. This is where the question of whether it's possible (or whether it makes sense) for us to actually provide the calling convention for the intrinsic (or whether the front-end has to explicitly state this).
> > > 
> > > Any suggestions on how to force the calling convention here?
> > Why do you want SysV64 calling convention?
> > 
> > I was trying to confirm that no registers will be clobbered unexpectedly, causing correctness issues.
> I thought that was the default calling convention for x86_64 on Linux, and it would greatly simplify the code in the sled to not have to put the registers in the correct registers expected by the function we are calling (`__xray_CustomEvent` is a trampoline). In my testing, for some reason the arguments we need might be placed in random registers. Like in the test, you will see that we need to move arguments around from the C calling convention, using different registers for the arguments, than the SysV64 calling convention (which the `__xray_CustomEvent` trampoline requires).
> 
> > I was trying to confirm that no registers will be clobbered unexpectedly, causing correctness issues.
> 
> Ah, yes -- AFAICT, the lowering of MachineInst's that are considered "call" instructions will preserve caller-saved registers before the call. Is there a way for me to verify that in the test more explicitly?
As long as it's correct, I'm fine.


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1075
+  // FIXME: Find another less hacky way do force the relative jump.
+  OutStreamer->EmitBytes("\xeb\x14");
+
----------------
dberris wrote:
> timshen wrote:
> > dberris wrote:
> > > timshen wrote:
> > > > Maybe jump to an "exit" symbol, and emit that symbol at the end of the sled?
> > > > 
> > > > See: https://github.com/llvm-mirror/llvm/blob/288180ac40ab050711298f42340233bd47916634/lib/Target/PowerPC/PPCAsmPrinter.cpp#L1076
> > > That doesn't quite work for x86 because the jump instruction emitted can vary (i.e. it's not always 2 bytes). The runtime requires us to be able to write two bytes atomically to enable/disable the jump. Since there's no way yet for us to force a specific jump instruction that will be preserved when the relaxation pass runs, for now we need to hard-code this static relative jump ourselves.
> > I'm not quite following - it's AsmPrinter, and there shouldn't be any lowering after that, is it right?
> > 
> > What is the "relaxation pass" you mentioned? Is it BranchRelaxation pass? But I think it runs before AsmPrinter?
> So the integrated assembler has an option (`-mrelax-all`) which will look for `jmp` instructions and attempt to relax those. The integrated assembler will run after all assembly is emitted. This works around that until we can flag certain jump instructions to never be relaxed.
\facepalm that's good to know. Thanks!


https://reviews.llvm.org/D27503