[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:
> > > 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.


http://reviews.llvm.org/D19046