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

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: docs/LangRef.rst:1408
@@ -1407,1 +1407,3 @@
     long as they do not significantly impact runtime performance.
+``"patchable-function"``
+    This attribute tells the code generator that the code
----------------
echristo wrote:
> Does this need to be a hard coded attribute? Why not something similar to the floating point ones while we're still working out things? Avoids needing to worry about bitcode reading/writing.
> Does this need to be a hard coded attribute?

Are you objecting to specifically documenting this attribute in the language reference?  I don't mind that at all, given that means less work for me. :)

>  Avoids needing to worry about bitcode reading/writing.

If I understood you correctly, we don't have to worry about that here either, since this is a string attribute.

================
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
----------------
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?

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2924
@@ +2923,3 @@
+
+  // We need to ensure that FirstMI is at least two bytes long (a short jump can
+  // be encoded in two bytes) and does not span a 16 byte boundary (to be
----------------
echristo wrote:
> Interesting. Is the idea here to avoid too much code growth? I'm assuming the performance of a pile of nops isn't that bad.
> 
> Also, are you just using the address of the symbol as the patchable address for the function?
> Interesting. Is the idea here to avoid too much code growth? I'm assuming the performance of a pile of nops isn't that bad.

Yes, we want to avoid too much code growth -- we have to do this for every function.  In an older scheme where we had a 5 byte nop in the function prologue unconditionally, we did see some performance impact on old amd64 chips.

> Also, are you just using the address of the symbol as the patchable address for the function?

Yes.  To redirect control away from `foo`, we basically patch `&foo`.


http://reviews.llvm.org/D19046