[PATCH] D45633: [XRay] Typed event logging intrinsic

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: include/llvm/IR/Intrinsics.td:900
+def int_xray_typedevent : Intrinsic<[], [llvm_i16_ty, llvm_ptr_ty, llvm_i32_ty],
+                                        [NoCapture<1>, ReadOnly<1>, IntrWriteMem]>;
 //===----------------------------------------------------------------------===//
----------------
kpw wrote:
> dberris wrote:
> > kpw wrote:
> > > I confess I copied IntrWriteMem from the custom event intrinsic without understanding why it is the right choice.
> > This is so that the optimisation passes cannot assume that the call to the intrinsic will not touch any global memory.
> What about IntrReadMem as well? IntrWriteMem is commented as
> 
> This intrinsic writes to unspecified memory, but does not
> read from memory, and has no other side effects. This means dead stores
> before calls to this intrinsics may be removed.
> 
Technically, the `ReadOnly<1>` attribute preserves the data pointed to, which is the minimal requirement for this intrinsic.

`IntrReadMem` signals that globals may be read, but the intrinsic itself really doesn't need this. This is so that we minimise the interference of the intrinsic when it's emitted in a function that may be in-lined. We want to say that this function acts like some sort of write barrier, but not a read barrier.


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1188
+    if (auto Op = MCIL.LowerMachineOperand(&MI, MI.getOperand(I))) {
+      // TODO: Is register only support adequate?
+      assert(Op->isReg() && "Only supports arguments in registers");
----------------
kpw wrote:
> dberris wrote:
> > Yes, mostly because we don't want to make the stack-based and/or constant global value handling special. It could be possible to support those too, but it's not as simple (and it's unclear whether we can do that with the same amount of bytes in the sled).
> Is there a better way we can support the contract that the intrinsic is invoked with register arguments than runtime assert?
The way we've forced the arguments to be in registers when lowering the pseudo-instruction is how we're telling the register allocators to do this for us. Until we come up with a way to force specific registers to be used at the MachineInstr level (which up until now I'm not sure is possible or desirable) we're going to be at the mercy of the default calling convention and therefore have to handle re-ordering the inputs per call-site.

The assertion here is meant to signal that we (us working on XRay) and the intervening optimisations on the machine instructions, have not messed up the fact that the arguments to the intrinsic call have to be in registers. Now there might be a way (similar to how the stack maps do it) to have much less interference with both typed events and custom events if we're able to record where the data is stored (could be global, could be in the stack already, could be a constant in a register already, etc), and emit the appropriate instrumentation point (with versioning and maybe some additional metadata).

We could do the more complicated thing which will reduce the register pressure from calling the intrinsic functions. But we'd need to measure the effects of that before committing to that course of action. :)

That's not to say we're not going to do it or that it will not be worth it -- I think we should do it, it's just a matter of "not now". But "not now" is closer to "in the next couple of months" rather than "never". :D


Repository:
  rL LLVM

https://reviews.llvm.org/D45633