[PATCH] Documentation for llvm.experimental.stackmap and llvm.experimental.patchpoint.

[Andrew Trick]

On Oct 18, 2013, at 11:52 PM, Sean Silva <silvas at purdue.edu> wrote:

> 
>  This is a lot better!
> 
> 
> ================
> Comment at: docs/Stackmaps.rst:126-141
> @@ +125,18 @@
> +
> +  uint32 : Reserved (header)
> +  uint32 : NumConstants
> +  int64  : Constants[]
> +  uint32 : NumRecords
> +  StkMapRecord[] {
> +    uint32 : PatchPoint ID
> +    uint32 : Instruction Offset
> +    uint16 : Reserved (record flags)
> +    uint16 : Num Locations
> +    Location[] {
> +      uint8  : Register | Direct | Indirect | Constant | ConstantIndex
> +      uint8  : Reserved (location flags)
> +      uint16 : Dwarf RegNum
> +      int32  : Offset
> +    }
> +  }
> +
> ----------------
> Is this notation meant to look like something? It's kind of groovy. But it seems inconsistent about whether the identifier goes first, or whether the type goes first. E.g., it seems like `Constants` should be `Constants[] : int64` for consistency with the other "arrays”.

I took the liberty of commenting the format in the way I personally find most readable. I'm not defining a language or complying with one. Ultimately, the format is specified by the unit tests. This is just a convenient reference. If there's strong objection, I'll replace it with C structs.

> Also, I feel like the `[]` should contain the name of the variable that gives the number of entries, just to be explicit. E.g. `StkMapRecord[NumRecords] {…`

Done.

> ================
> Comment at: docs/Stackmaps.rst:14
> @@ +13,3 @@
> +address. LLVM emits stackmap data into the object code within a
> +__STACKMAP section. This stackmap data contains a record for each
> +stackmap. The record stores the stackmap's instruction address and
> ----------------
> This section name sounds like a Darwin-ism; surely that's not the name on ELF (`.llvm.stackmap`?). Or maybe adjust the wording to be "currently this functionality is only available on Darwin, and information is emitted into the section `__STACKMAP`”

Done.

> ================
> Comment at: docs/Stackmaps.rst:15-17
> @@ +14,5 @@
> +__STACKMAP section. This stackmap data contains a record for each
> +stackmap. The record stores the stackmap's instruction address and
> +contains a entry for each mapped value. Each entry encodes a
> +value's location as a register, stack offset, or constant.
> +
> ----------------
> It seems like this overlaps somewhat with the information contained in debug info. Could you maybe compare/contrast them?

It's natural to compare them, but when it comes down to it, the two things are very fundamentally different. I added a Stackmap Usage section, trying not to go to far onto a tangent.

> ================
> Comment at: docs/Stackmaps.rst:61-62
> @@ +60,4 @@
> +
> +The first argument is an identifier to be encoded within the stackmap. The second argument is the number of shadow bytes following the
> +intrinsic. The variable number of arguments after that are the live
> +values.
> ----------------
> "shadow bytes" still hasn't been defined. All it needs is probably a simple ascii diagram showing the conceptual layout of the instruction stream, and clearly indicating what "N shadow bytes" corresponds to (e.g., does it include or exclude the size of the an instruction?).

Done. Sort of. I added a code block.

> ================
> Comment at: docs/Stackmaps.rst:110-115
> @@ +109,8 @@
> +arguments are lowered according to the calling convention specified at
> +the callsite of the intrinsic. The location of the arguments are not
> +normally recorded in the stackmap. However, special stack-map specific
> +calling conventions can force the argument locations to be
> +recorded. The patchpoint also emits nops to cover at least <numBytes>
> +of instruction encoding space. Hence, the client must ensure that
> +<numBytes> is enough to encode a call to the target address on the
> +supported targets. The runtime may patch the code emitted for the
> ----------------
> I think this would benefit from an ASCII diagram too.

Added a code block here too.

-Andy