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

[Eric Christopher via llvm-commits]

I don't have anything to add to dberris's comments. One reply inline.

On Thu, Apr 14, 2016, 11:00 PM Sanjoy Das <sanjoy at playingwithpointers.com>
wrote:

> sanjoy added inline comments.
>
> ================
> 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
> ----------------
> dberris wrote:
> > 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
> Thanks!  I think I'll go with `"prologue-short-redirect"`.
>
> ================
> Comment at: lib/CodeGen/PatchableFunction.cpp:44-47
> @@ +43,6 @@
> +  Attribute PatchAttr =
> MF.getFunction()->getFnAttribute("patchable-function");
> +  StringRef PatchType = PatchAttr.getValueAsString();
> +
> +  assert(PatchType == "prologue-hotpatch-compact" && "Only possibility
> today!");
> +  (void)PatchType;
> +
> ----------------
> echristo wrote:
> > Can merge all of this.
> Did not quite understand what you meant here. :)
>

Ah. You can fold the stringref call into the assert and get rid of the void
cast?



> ================
> 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()) {
> ----------------
> dberris wrote:
> > Have you considered inserting a pseudo instruction that gets translated
> instead when emitting the assembler?
> That's a good idea, let me give that a try.
>
> ================
> Comment at: lib/Target/X86/X86FrameLowering.h:207-209
> @@ -206,1 +206,5 @@
> +
> +  void makeFunctionProloguePatchable(
> +      MachineFunction &MF,
> +      TargetFrameLowering::PatchablePrologueKind PPK) const override;
>  };
> ----------------
> dberris wrote:
> > 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.
> That's a good point, will do.
>
>
> http://reviews.llvm.org/D19046
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160415/ef203a00/attachment.html>