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


http://reviews.llvm.org/D19046