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

[Dean Michael Berris via llvm-commits]

dberris added inline comments.

================
Comment at: lib/Target/X86/X86MCInstLower.cpp:948-949
@@ +947,4 @@
+    } else {
+      EmitNops(*OutStreamer, MinSize, Subtarget->is64Bit(), getSubtargetInfo(),
+               /* OnlyOneNop = */ true);
+    }
----------------
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?


http://reviews.llvm.org/D19046