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

[Dean Michael Berris via llvm-commits]

dberris added a comment.

PS. First review in LLVM, please be gentle? :)


================
Comment at: docs/LangRef.rst:1415
@@ +1414,3 @@
+
+     * ``"prologue-hotpatch-compact"`` - This style of patchable
+       function is intended to support patching a function prologue to
----------------
sanjoy wrote:
> echristo wrote:
> > Perhaps a different name for it? hotpatch-compact isn't particularly enlightening without the description. Is the "compact" because it only handles the small code model? It might be best to talk about the option in an architecture neutral way and then explain the particular implementation in a cpu specific section below for it.
> "compact" as in "two bytes".  I've tried to not mention any arch-specific details here (while avoid making things vague).  Can you be more specific about how I can make this description less arch specific?
My thought here is something that's recognisable. Consider things like:

- compact-redirect-prologue
- compact-rewrite-prologue
- prologue-short-redirect
- short-prologue-redirect
- short-prologue-rewrite

If you intend to use "hotpatch" as a namespace of sorts (if there will be more later), something like:

- hotpatch-short-prologue
- hotpatch-prologue-small

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2928-2930
@@ +2927,5 @@
+
+  // Programmatically getting the size of an x86 instruction at the MachineInstr
+  // level is difficult.  So we instead hard-code a list of instructions that
+  // are commonly expected to appear as the first instruction in MF.
+  switch (FirstMI.getOpcode()) {
----------------
Have you considered inserting a pseudo instruction that gets translated instead when emitting the assembler?

================
Comment at: lib/Target/X86/X86FrameLowering.h:207-209
@@ -206,1 +206,5 @@
+
+  void makeFunctionProloguePatchable(
+      MachineFunction &MF,
+      TargetFrameLowering::PatchablePrologueKind PPK) const override;
 };
----------------
Is this intended to only handle prologues? Consider making this a single entry point, naming it something like `makeFunctionPatchable(...)`. There may be other places where the patch-sleds could be inserted (before calls to functions, before entering loops, before returning, etc.) and it would be really great if this wasn't tied just to the prologue.


http://reviews.llvm.org/D19046