<div dir="ltr">I don't have anything to add to dberris's comments. One reply inline. <br>
</div><span>
</span><br><div class="gmail_quote"><div dir="ltr">On Thu, Apr 14, 2016, 11:00 PM Sanjoy Das <<a href="mailto:sanjoy@playingwithpointers.com">sanjoy@playingwithpointers.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">sanjoy added inline comments.<br>
<br>
================<br>
Comment at: docs/LangRef.rst:1415<br>
@@ +1414,3 @@<br>
+<br>
+     * ``"prologue-hotpatch-compact"`` - This style of patchable<br>
+       function is intended to support patching a function prologue to<br>
----------------<br>
dberris wrote:<br>
> sanjoy wrote:<br>
> > echristo wrote:<br>
> > > 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.<br>
> > "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?<br>
> My thought here is something that's recognisable. Consider things like:<br>
><br>
> - compact-redirect-prologue<br>
> - compact-rewrite-prologue<br>
> - prologue-short-redirect<br>
> - short-prologue-redirect<br>
> - short-prologue-rewrite<br>
><br>
> If you intend to use "hotpatch" as a namespace of sorts (if there will be more later), something like:<br>
><br>
> - hotpatch-short-prologue<br>
> - hotpatch-prologue-small<br>
Thanks!  I think I'll go with `"prologue-short-redirect"`.<br>
<br>
================<br>
Comment at: lib/CodeGen/PatchableFunction.cpp:44-47<br>
@@ +43,6 @@<br>
+  Attribute PatchAttr = MF.getFunction()->getFnAttribute("patchable-function");<br>
+  StringRef PatchType = PatchAttr.getValueAsString();<br>
+<br>
+  assert(PatchType == "prologue-hotpatch-compact" && "Only possibility today!");<br>
+  (void)PatchType;<br>
+<br>
----------------<br>
echristo wrote:<br>
> Can merge all of this.<br>
Did not quite understand what you meant here. :)<br></blockquote></div><div><br></div><div>Ah. You can fold the stringref call into the assert and get rid of the void cast?</div><div><br></div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Target/X86/X86FrameLowering.cpp:2928-2930<br>
@@ +2927,5 @@<br>
+<br>
+  // Programmatically getting the size of an x86 instruction at the MachineInstr<br>
+  // level is difficult.  So we instead hard-code a list of instructions that<br>
+  // are commonly expected to appear as the first instruction in MF.<br>
+  switch (FirstMI.getOpcode()) {<br>
----------------<br>
dberris wrote:<br>
> Have you considered inserting a pseudo instruction that gets translated instead when emitting the assembler?<br>
That's a good idea, let me give that a try.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86FrameLowering.h:207-209<br>
@@ -206,1 +206,5 @@<br>
+<br>
+  void makeFunctionProloguePatchable(<br>
+      MachineFunction &MF,<br>
+      TargetFrameLowering::PatchablePrologueKind PPK) const override;<br>
 };<br>
----------------<br>
dberris wrote:<br>
> 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.<br>
That's a good point, will do.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D19046" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19046</a><br>
<br>
<br>
<br>
</blockquote></div>