[PATCH] D19046: Introduce a "patchable-function" function attribute

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: lib/Target/X86/X86MCInstLower.cpp:948-949
@@ +947,4 @@
+    } else {
+      EmitNops(*OutStreamer, MinSize, Subtarget->is64Bit(), getSubtargetInfo(),
+               /* OnlyOneNop = */ true);
+    }
----------------
dberris wrote:
> sanjoy wrote:
> > dberris wrote:
> > > sanjoy wrote:
> > > > dberris wrote:
> > > > > So in this branch, MinSize != 2 or Opcode != X86::PUSH64r.
> > > > > 
> > > > > Question: If you check instead whether the function where MI is included in had the correct type of patchable-function attribute, and see that the nops being added is less than 2, you can assert and say this is actually a bug in a higher implementation detail? i.e. this would be a bug in the insertion of this instruction.
> > > > > 
> > > > > This way you wouldn't need to touch EmitNops.
> > > > > Question: If you check instead whether the function where MI is included in had the correct type of patchable-function attribute, and see that the nops being added is less than 2, you can assert and say this is actually a bug in a higher implementation detail? i.e. this would be a bug in the insertion of this instruction.
> > > > 
> > > > Do you mean something like:
> > > > 
> > > > ```
> > > > assert(MinSize < 2 && !MF.getPatchableFnType() == "prologue-short-redirect");
> > > > ```
> > > > 
> > > > I'd say that is an incorrect layering.  It feels cleaner for MC to not have to know why the `PATCHABLE_OP` of a certain variety is present.  It should just understand its end of the contract of how it needs to lower `PATCHABLE_OP`.
> > > > I'd say that is an incorrect layering. It feels cleaner for MC to not have to know why the PATCHABLE_OP of a certain variety is present. It should just understand its end of the contract of how it needs to lower PATCHABLE_OP.
> > > 
> > > Consider the true branch of this if-statement though -- it already feels like it's already bleeding details in based on what a short 'prologue-short-redirect' patchable function attribute is already expecting. I'd say we're already breaking some layering guidelines here. :D
> > I don't think those two are the same things.  One is saying "in this specific case I know can do better" (and you can extend the logic later by not just handling push'es but also returns, for instance), and the other is saying "this is the only case I support".
> > 
> > I'm not opposed to having `PATCHABLE_OP` specifically only work with `"prologue-short-redirect"`, but then I'd rather have it not have a `minsize` operand at all.  Do you think that will be cleaner?  (I could go either way on this -- no strong preferences).
> > 
> > Actually, now that I think about it, what I dislike about the current scheme (i.e. this patch) is that only the `minsize` == `2` case is tested, so from just a testing / code coverage POV, removing `minsize` sounds slightly better.
> I think minsize still makes sense, but I'm thinking that there's two inputs here really:
> 
> 1. That there are instructions to be placed here with a given minimum size. Note that this could be not just a single instruction, or could be treated just as a placeholder.
> 2. That the semantics of what kinds of instructions will be emitted would be based on the type of function patching is supported. In this case you're implementing 'prologue-short-redirect' which expects certain kinds of operations to be valid where a `PATCHABLE_OP` appears.
> 
> Which is why I think looking at the attribute that defines the semantics of the `PATCHABLE_OP` still makes sense at this level. It allows us to make decisions of what's valid or not valid based on the attribute provided on the function. And I think this is the correct layer to make that decision, because this is where we're actually generating the instructions.
> 
> Does that reasoning make sense?
> I think minsize still makes sense, but I'm thinking that there's two
> inputs here really:
>
> That there are instructions to be placed here with a given minimum
> size. Note that this could be not just a single instruction, or could
> be treated just as a placeholder.
>
> That the semantics of what kinds of instructions will be emitted would
> be based on the type of function patching is supported. In this case
> you're implementing 'prologue-short-redirect' which expects certain
> kinds of operations to be valid where a PATCHABLE_OP appears.
>
> Which is why I think looking at the attribute that defines the
> semantics of the PATCHABLE_OP still makes sense at this level. It
> allows us to make decisions of what's valid or not valid based on the
> attribute provided on the function. And I think this is the correct

I've been thinking about this as: an instance of `PATCHABLE_OP` is
(should be) self contained with regards to what needs to be emitted
when MC encounters one.  Right now, as you said, emitting a
`PATCHABLE_OP` involves ensuring two things:

 1. There is *one* instruction of at least `MinSize` bytes at the
    place in the instruction stream the `PATCHABLE_OP` appeared at.

 2. The set of instructions emitted as part of lowering the
    `PATCHABLE_OP` pseudo instruction is "equivalent" (i.e. has the
    same effect on the CPU state, modulo the delta by which the
    instruction pointer is advanced) to the instruction bundled with
    `PATCHABLE_OP`.

(I could be more explicit about this in comment over PATCHABLE_OP --
let me know if you think that will help)

This is all that a `PATCHABLE_OP` implies.  Any optimizations that
happens on top of (1) and (2) are strictly optional, and cannot tread
beyond what is allowed by (1) and (2).

If I understand you correctly, you're saying `PATCHABLE_OP` is **not**
self contained, but interpreting MC needs to do when it sees a
`PATCHABLE_OP` depends on what attributes the containing function has.
I still don't think this is correct (assuming I haven't misrepresented
your position): unless we gain something by by coupling function
attributes with `PATCHABLE_OP`, I'd rather have these de-coupled
(i.e. have `PATCHABLE_OP` be the mechanism by which the
`"patchable-function"` policy is implemented).  For instance, a
potential use case for `PATCHABLE_OP` is to make //some// call sites
patchable, and one way to do that is via call site attributes.
Without making `PATCHABLE_OP` self sufficient we'd have to resort to
walking back to the relevant (IR level) call instruction (which may be
difficult to locate), in addition to checking the function attributes.
What if after this we want to add a third source for `PATCHABLE_OP`
(an intrinsic, say)?  The complexity of lowering `PATCHABLE_OP`,
unless it is self sufficient, will scale linearly with the number of
ways we can generate one.


> layer to make that decision, because this is where we're actually
> generating the instructions.
>
> Does that reasoning make sense?


http://reviews.llvm.org/D19046