[RS4GC] Refactoring to make a later change easier, NFCI

[sanjoy]

Summary

These non-semantic changes will help make a later change adding
support for deopt operand bundles more streamlined.

[reames]

LGTM w/comments addressed.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1354

For clarity, I might call this OldSP.

1357

const?

1359

This doesn't look right. You're converting a bitfield to an emum representing what the bits mean. I think you want the full bitfield.

1374

In a separate change, might be better to use the name of the original statepoint. That will cause test churn, so please do separately if at all.

test/Transforms/RewriteStatepointsForGC/relocation.ll

97

I'm not seeing why this changed? Is this a non-determinism? Or did something in your patch effect ordering?

[sanjoy]

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1357

I'd then have to remove the const in the later semantic patch, which would make that diff larger.

test/Transforms/RewriteStatepointsForGC/relocation.ll

97

It looks like there is a "semantic" change in this refactoring. I'm
not a 100% confident that I understand what's going on, but it looks
like CreateGCStatepointCall creates a named call instruction and
then inserts the said call instruction, whereas CreateCall
creates a nameless instruction, inserts it, and then calls setName
on it. This changes the number of times LastUnique is incremented
for the Function s ValueSymbolTable because calling setName on
an unattached instruction does not update the symbol table (since
there is no symbol table to update).

This does not really matter though -- only the stringified name is
affected, the bits of IR that matter should remain the same.

include/llvm/IR/Statepoint.h

public:
 
   typename CallSiteTy::arg_iterator gc_args_begin() const {
     return vm_state_end();
   }
   typename CallSiteTy::arg_iterator gc_args_end() const {
     return getCallSite().arg_end();
   }
 
+  unsigned gcArgsStartIdx() const {
+    return gc_args_begin() - getInstruction()->op_begin();
+  }
+
   /// range adapter for gc arguments
   iterator_range<arg_iterator> gc_args() const {
     return iterator_range<arg_iterator>(gc_args_begin(), gc_args_end());
   }
 
   /// Get list of all gc reloactes linked to this statepoint
   /// May contain several relocations for the same base/derived pair.
   /// For example this could happen due to relocations on unwinding

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

makeStatepointExplicitImpl(const CallSite CS, /* to replace */
   assert(isStatepoint(CS) &&
          "This method expects to be rewriting a statepoint");
 
   BasicBlock *BB = CS.getInstruction()->getParent();
   Function *F = BB->getParent();
   Module *M = F->getParent();
   assert(M && "must be set");
 
-  // We're not changing the function signature of the statepoint since the gc
-  // arguments go into the var args section.
-  Function *GCStatepointDecl = CS.getCalledFunction();
-
   // Then go ahead and use the builder do actually do the inserts.  We insert
   // immediately before the previous instruction under the assumption that all
   // arguments will be available here.  We can't insert afterwards since we may
   // be replacing a terminator.
   Instruction *InsertBefore = CS.getInstruction();
   IRBuilder<> Builder(InsertBefore);
 
-  // Copy all of the arguments from the original statepoint - this includes the
-  // target, call args, and deopt args
-  SmallVector<llvm::Value *, 64> Args;
-  Args.insert(Args.end(), CS.arg_begin(), CS.arg_end());
-  // TODO: Clear the 'needs rewrite' flag
-
-  // Add all the pointers to be relocated (gc arguments) and capture the start
-  // of the live variable list for use in the gc_relocates
-  const int LiveStartIdx = Args.size();
-  Args.insert(Args.end(), LiveVariables.begin(), LiveVariables.end());
+  Statepoint SP(CS);
+
+  ArrayRef<Value *> GCArgs(LiveVariables);
+  uint64_t StatepointID = SP.getID();
+  uint32_t NumPatchBytes = SP.getNumPatchBytes();
+  StatepointFlags Flags = StatepointFlags(SP.getFlags());
+
+  ArrayRef<Use> CallArgs(SP.arg_begin(), SP.arg_end());
+  ArrayRef<Use> DeoptArgs(SP.vm_state_begin(), SP.vm_state_end());
+  ArrayRef<Use> TransitionArgs(SP.gc_transition_args_begin(),
+                               SP.gc_transition_args_end());
+  Value *CallTarget = SP.getCalledValue();
 
   // Create the statepoint given all the arguments
   Instruction *Token = nullptr;
   AttributeSet ReturnAttrs;
   if (CS.isCall()) {
     CallInst *ToReplace = cast<CallInst>(CS.getInstruction());
-    CallInst *Call =
-        Builder.CreateCall(GCStatepointDecl, Args, "safepoint_token");
+    CallInst *Call = Builder.CreateGCStatepointCall(
+        StatepointID, NumPatchBytes, CallTarget, Flags, CallArgs,
+        TransitionArgs, DeoptArgs, GCArgs, "safepoint_token");
+
     Call->setTailCall(ToReplace->isTailCall());
     Call->setCallingConv(ToReplace->getCallingConv());
 
     // Currently we will fail on parameter attributes and on certain
     // function attributes.
     AttributeSet NewAttrs = legalizeCallAttributes(ToReplace->getAttributes());
     // In case if we can handle this set of attributes - set up function attrs
     // directly on statepoint and return attrs later for gc_result intrinsic.
     Call->setAttributes(NewAttrs.getFnAttributes());
     ReturnAttrs = NewAttrs.getRetAttributes();
 
     Token = Call;
 
     // Put the following gc_result and gc_relocate calls immediately after the
     // the old call (which we're about to delete)
     assert(ToReplace->getNextNode() && "Not a terminator, must have next!");
     Builder.SetInsertPoint(ToReplace->getNextNode());
     Builder.SetCurrentDebugLocation(ToReplace->getNextNode()->getDebugLoc());
   } else {
     InvokeInst *ToReplace = cast<InvokeInst>(CS.getInstruction());
 
     // Insert the new invoke into the old block.  We'll remove the old one in a
     // moment at which point this will become the new terminator for the
     // original block.
-    InvokeInst *Invoke =
-        InvokeInst::Create(GCStatepointDecl, ToReplace->getNormalDest(),
-                           ToReplace->getUnwindDest(), Args, "statepoint_token",
-                           ToReplace->getParent());
+    InvokeInst *Invoke = Builder.CreateGCStatepointInvoke(
+        StatepointID, NumPatchBytes, CallTarget, ToReplace->getNormalDest(),
+        ToReplace->getUnwindDest(), Flags, CallArgs, TransitionArgs, DeoptArgs,
+        GCArgs, "statepoint_token");
+
     Invoke->setCallingConv(ToReplace->getCallingConv());
 
     // Currently we will fail on parameter attributes and on certain
     // function attributes.
     AttributeSet NewAttrs = legalizeCallAttributes(ToReplace->getAttributes());
     // In case if we can handle this set of attributes - set up function attrs
     // directly on statepoint and return attrs later for gc_result intrinsic.
     Invoke->setAttributes(NewAttrs.getFnAttributes());
if (CS.isCall()) {
 
     // Extract second element from landingpad return value. We will attach
     // exceptional gc relocates to it.
     Instruction *ExceptionalToken =
         cast<Instruction>(Builder.CreateExtractValue(
             UnwindBlock->getLandingPadInst(), 1, "relocate_token"));
     Result.UnwindToken = ExceptionalToken;
 
+    const unsigned LiveStartIdx = Statepoint(Token).gcArgsStartIdx();
     CreateGCRelocates(LiveVariables, LiveStartIdx, BasePtrs, ExceptionalToken,
                       Builder);
 
     // Generate gc relocates and returns for normal block
     BasicBlock *NormalDest = ToReplace->getNormalDest();
     assert(!isa<PHINode>(NormalDest->begin()) &&
            NormalDest->getUniquePredecessor() &&
            "can't safely insert in this block!");
#endif
   // Update the gc.result of the original statepoint (if any) to use the newly
   // inserted statepoint.  This is safe to do here since the token can't be
   // considered a live reference.
   CS.getInstruction()->replaceAllUsesWith(Token);
 
   Result.StatepointToken = Token;
 
   // Second, create a gc.relocate for every live variable
+  const unsigned LiveStartIdx = Statepoint(Token).gcArgsStartIdx();
   CreateGCRelocates(LiveVariables, LiveStartIdx, BasePtrs, Token, Builder);
 }
 
 namespace {
 struct NameOrdering {
   Value *Base;
   Value *Derived;
 

test/Transforms/RewriteStatepointsForGC/relocation.ll

; CHECK: gc.relocate
   %safepoint_token1 = call i32 (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @foo, i32 0, i32 0, i32 0, i32 0)
   br label %join
 
 join:
 ; We need to end up with a single relocation phi updated from both paths 
 ; CHECK-LABEL: join:
 ; CHECK: phi i8 addrspace(1)*
 ; CHECK-DAG: [ %arg.relocated, %if_branch ]
-; CHECK-DAG: [ %arg.relocated4, %else_branch ]
+; CHECK-DAG: [ %arg.relocated3, %else_branch ]
 ; CHECK-NOT: phi
   call void (i8 addrspace(1)*) @some_call(i8 addrspace(1)* %arg)
   ret void
 }
 
 ; Make sure a use in a statepoint gets properly relocated at a previous one.  
 ; This is basically just making sure that statepoints aren't accidentally 
 ; treated specially.