[PATCH] Implement function prefix data as an IR feature.

[Peter Collingbourne]

On Mon, Jul 29, 2013 at 10:24:45PM -0700, Chris Lattner wrote:
> 
> On Jul 20, 2013, at 6:40 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
> 
> > Previous discussion:
> > http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-July/063909.html
> > 
> > http://llvm-reviews.chandlerc.com/D1191
> 
> 
> This is gross, but I'm not morally opposed to this approach. :-)
> 
> Some questions:
>  - Is this actually sufficient to implement support for GHC's TNTC optimization?

I believe that compliance with the existing TNTC ABI would require
the symbol to appear between the data and the code; see below.

>  What else is it relevant for?

This could be used for any form of runtime function metadata.
For example, I've used this to implement the function type checker
for UBSan that I mentioned in [2].

>  - Given that this is completely target specific, is there any value in having some targets (e.g. not darwin) emit the blob *before* the function's label?  This would avoid the need to conditional branch around the blob.

Yes.  I don't think this should be target-dependent behaviour, though.
The way I think this should be done is through global symbol offsets
[1].

We can't use this layout in cases where the metadata is optional,
as a metadata load behind the symbol may segfault if it lies on
a section boundary.  So it wouldn't work for UBSan, for example,
as an arbitrary indirect call may be calling an uninstrumented function.

Now that you've brought it up, is it true that Darwin prohibits this
kind of thing?  After Duncan brought it up I wrote a small test program
(load.cpp) which I compiled (loadcpp.orig.s) and modified (loadcpp.s).
The resulting program worked as I expected.  Maybe it only fails to
work in cases which I haven't considered.

>  - What is the behavior of inlining and other IPO w.r.t. this?  If this requires disabling all inlining and other IPO, then we need to know how to model it and I'd like to know how frontends are going to use it.

I designed this feature so that a call to a function with prefix data
has the same semantics as a call to a function without, so IPO passes
shouldn't be affected.  The prefix data only has semantics when the
function pointer is bitcasted and loaded from.  This is because of
the requirement on the prefix data that it must transfer control to
the point immediately succeeding the prefix data, without performing
any other visible action.  (This property enables a number of other
optimisations; e.g. we can drop prefix data from functions with
internal linkage when the prefix data is provably never accessed,
or demote an internal linkage function with prefix data to a global
constant if the prefix data is read but the function is provably
never called.)

>  - In Function.h, this is really unfortunate:
> 
> +  bool hasPrefixData() const {
> +    return getNumOperands() != 0;
> +  }
> +
> +  Constant *getPrefixData() const {
> +    assert(hasPrefixData());
> +    return cast<Constant>(Op<0>());
> +  }
> +
> +  void setPrefixData(Constant *PrefixData);
> 
> We want to keep the sizeof(Function) as small as possible, and this feature will almost never be used.  I'd much rather see this be stored with a single bit in Function::SubclassOptionalData (from Value) that indicates "has entry in an on-the-side hash table" (and also is used to implement hasPrefixData()) and store the Constant* in the hashtable.

What if someone RAUWs one of the constants?  Wouldn't we need to
introduce a new User subclass that would be stored in the hash table
and hold the prefix data as an operand?

>  - Should this be modeled as some kind of crazy attribute?  That gives you a *different* place to unique it into and also would avoid punishing clients that aren't using it.

I initially wanted to implement this as an attribute, but I realised
that we'd have the same RAUW problem I mentioned above.  It may well be
better to do the work to allow attributes to hold Values rather than
implementing something for prefix data that can't be reused.  I also
don't know how difficult maintaining the bit in SubclassOptionalData
would be (or if it's worth maintaining it) if we store this as an
attribute.  I'd like to get Bill's views on this as well.

(My gut feeling is that it would be better to take the extra storage
hit rather than introduce this kind of complexity.)

Thanks,
-- 
Peter

[1] http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-April/061511.html
[2] http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-July/063909.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: load.cpp
Type: text/x-c++src
Size: 120 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130730/dfd4a388/attachment.cpp>
-------------- next part --------------
	.section	__TEXT,__text,regular,pure_instructions
	.globl	_main
	.align	4, 0x90
_main:                                  ## @main
	.cfi_startproc
## BB#0:
	pushq	%rbp
Ltmp2:
	.cfi_def_cfa_offset 16
Ltmp3:
	.cfi_offset %rbp, -16
	movq	%rsp, %rbp
Ltmp4:
	.cfi_def_cfa_register %rbp
	subq	$32, %rsp
	leaq	L_.str(%rip), %rdi
	movq	__Z1fv at GOTPCREL(%rip), %rax
	movl	$0, -4(%rbp)
	movq	%rax, -16(%rbp)
	movq	-16(%rbp), %rax
	movl	-4(%rax), %esi
	movb	$0, %al
	callq	_printf
	movl	$0, %esi
	movl	%eax, -20(%rbp)         ## 4-byte Spill
	movl	%esi, %eax
	addq	$32, %rsp
	popq	%rbp
	ret
	.cfi_endproc

	.section	__TEXT,__textcoal_nt,coalesced,pure_instructions
	.globl	__Z1fv
	.weak_definition	__Z1fv
	.align	4, 0x90
__Z1fv:                                 ## @_Z1fv
	.cfi_startproc
## BB#0:
	pushq	%rbp
Ltmp7:
	.cfi_def_cfa_offset 16
Ltmp8:
	.cfi_offset %rbp, -16
	movq	%rsp, %rbp
Ltmp9:
	.cfi_def_cfa_register %rbp
	popq	%rbp
	ret
	.cfi_endproc

	.section	__TEXT,__cstring,cstring_literals
L_.str:                                 ## @.str
	.asciz	 "%d\n"


.subsections_via_symbols
-------------- next part --------------
	.section	__TEXT,__text,regular,pure_instructions
	.globl	_main
	.align	4, 0x90
_main:                                  ## @main
	.cfi_startproc
## BB#0:
	pushq	%rbp
Ltmp2:
	.cfi_def_cfa_offset 16
Ltmp3:
	.cfi_offset %rbp, -16
	movq	%rsp, %rbp
Ltmp4:
	.cfi_def_cfa_register %rbp
	subq	$32, %rsp
	leaq	L_.str(%rip), %rdi
	movq	__Z1fv at GOTPCREL(%rip), %rax
	movl	$0, -4(%rbp)
	movq	%rax, -16(%rbp)
	movq	-16(%rbp), %rax
	movl	-4(%rax), %esi
	movb	$0, %al
	callq	_printf
	movl	$0, %esi
	movl	%eax, -20(%rbp)         ## 4-byte Spill
	movl	%esi, %eax
	addq	$32, %rsp
	popq	%rbp
	ret
	.cfi_endproc

	.section	__TEXT,__textcoal_nt,coalesced,pure_instructions
	.globl	__Z1fv
	.weak_definition	__Z1fv
	.align	4, 0x90
dummy:                                 ## @_Z1fv
__Z1fv=dummy+4
	.long 42
	.cfi_startproc
## BB#0:
	pushq	%rbp
Ltmp7:
	.cfi_def_cfa_offset 16
Ltmp8:
	.cfi_offset %rbp, -16
	movq	%rsp, %rbp
Ltmp9:
	.cfi_def_cfa_register %rbp
	popq	%rbp
	ret
	.cfi_endproc

	.section	__TEXT,__cstring,cstring_literals
L_.str:                                 ## @.str
	.asciz	 "%d\n"


.subsections_via_symbols