[LangRef] Require elementtype attribute for gc.statepoint intrinsic

[nikic]

Summary

The gc.statepoint intrinsic currently determines the target function type based on the pointer element type of the argument. In order to support opaque pointers, require that the argument is annotated with an elementtype attribute.

This is just the LangRef change, this will require followup changes to update tests, the IR verifier and the statepoint code.

Here's an example of the change:

; Before:
  %safepoint_token = tail call token (i64, i32, i1 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_i1f(i64 0, i32 0, i1 ()* @return_i1, i32 0, i32 0, i32 0, i32 0)
; After:
  %safepoint_token = tail call token (i64, i32, i1 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_i1f(i64 0, i32 0, i1 ()* elementtype(i1 ()) @return_i1, i32 0, i32 0, i32 0, i32 0)
; After with opaque pointers:
  %safepoint_token = tail call token (i64, i32, i1 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0(i64 0, i32 0, ptr elementtype(i1 ()) @return_i1, i32 0, i32 0, i32 0, i32 0)

[dblaikie]

Generally sounds OK to me. (not sure what we do historically for these sort of changes - but I'd guess the documentation usually comes after the functionality (or in the same patch) rather than before - it might be relevant to see what test case cleanup looks like, etc, to evaluate the direction. But I don't mind too much - may be worth waiting to commit this until a few of the other patches in the series are available/reviewed)

[reames]

What piece of code are you trying to remove? I don't think we need this change unless I'm missing something. The callee return type should already be encoded in the signature of the associated gc.result.

I definitely would not want this landing without the associated builder and test changes.

[nikic]

In D117890#3261829, @reames wrote:

What piece of code are you trying to remove? I don't think we need this change unless I'm missing something. The callee return type should already be encoded in the signature of the associated gc.result.

Yes, the callee return type is the important bit. Thanks for the suggestion on using the gc.result type for this purpose. Something I'm not quite clear on is whether a gc.statepoint with a non-void callee is required to also have a gc.result use? And if not, whether it is fine to simply lower the call with a void result instead?

I definitely would not want this landing without the associated builder and test changes.

I'm mainly interested in feedback on the general direction here, before I spend a lot of time updating tests.

[nikic]

@reames I tried using the gc.result type (https://gist.github.com/nikic/703794bdf8ab04c4a4ca40c4145a1f90), but this did result in codegen changes. In particular, if there is no gc.result use, but the function is non-void, then we no longer have the implicit-def dead $eax on the statepoint (see the last two test changes).

I'm not familiar with the area, but I assume that would be incorrect, as we lose the knowledge that the register may be clobbered by the call?

[reames]

@nikic I will take an action item to remove the use of the pointer type. I'm pretty sure I can, just need to sit down and remind myself how the code works again. This week is absolutely *crazy* for me in terms of personal life, so it probably won't happen until a week or so out.

[reames]

Ok, I got back to this today. Short version, Nikita was right all along, and this patch is now LGTM.

Longer version...

I landed 57cf29a and 6e4f7c08 which removed two uses of getActualReturnType. These two are safe to drive from the type of the gc.result, since the code is unreachable if there isn't a gc.result present.

However, the remaining case can't be removed. Specifically, we need to know the result type to know which phys regs are being clobbered by the call; that's a function of calling convention and call site signature (i.e. return type).

Given that, we have two options:

1. Require that the optimize preserve a gc.result if initially present. This requires us not to recognize such calls as dead, and generally complicates the code to preserve optimizations we get for "free" today.
2. We find a way to express the type in the callsite again.

This patch implements (2), and does so in what appears to be a newly idiomatic way for opaque pointers. (I'd missed that previously.) As such, this looks like an utterly reasonable solution.

@nikic, are you going to follow up with the code changes to RS4GC and the IRBuilder interface? Or would you like me to do so?

llvm/docs/LangRef.rst

 
 Syntax:
 """""""
 
 ::
 
       declare token
         @llvm.experimental.gc.statepoint(i64 <id>, i32 <num patch bytes>,
-                       func_type <target>,
+                       func_type* elementtype(func_type) <target>,
                        i64 <#call args>, i64 <flags>,
                        ... (call parameters),
                        i64 0, i64 0)
 
 Overview:
 """""""""
 
 The statepoint intrinsic represents a call which is parse-able by the

 calling sequence specific to their runtime before executing the
 generated machine code.  There are no guarantees with respect to the
 alignment of the nop sequence.  Unlike :doc:`StackMaps` statepoints do
 not have a concept of shadow bytes.  Note that semantically the
 statepoint still represents a call or invoke to 'target', and the nop
 sequence after patching is expected to represent an operation
 equivalent to a call or invoke to 'target'.
 
-The 'target' operand is the function actually being called.  The
-target can be specified as either a symbolic LLVM function, or as an
-arbitrary Value of appropriate function type.  Note that the function
-type must match the signature of the callee and the types of the 'call
-parameters' arguments.
+The 'target' operand is the function actually being called. The operand
+must have an :ref:`elementtype <attr_elementtype>` attribute specifying
+the function type of the target. The target can be specified as either
+a symbolic LLVM function, or as an arbitrary Value of pointer type. Note
+that the function type must match the signature of the callee and the
+types of the 'call parameters' arguments.
 
 The '#call args' operand is the number of arguments to the actual
 call.  It must exactly match the number of arguments passed in the
 'call parameters' variable length section.
 
 The 'flags' operand is used to specify extra information about the
 statepoint. This is currently only used to mark certain statepoints
 as GC transitions. This operand is a 64-bit integer with the following