[Statepoint Lowering] Handle the case with several gc.result

[skatkov]

Summary

Recently gc.result has been marked with readnone instead of readonly and
this opens a door for different optimization to duplicate gc.result.
Statepoint lowering is not ready to see several gc.results.
The problem appears when there are gc.results with one located in the same
basic block and another located in other basic block.
In this case we need both export VR and fill local setValue.

Note that this case is not sufficient optimization done before CodeGen.
It is evident that local gc.result dominates all other gc.results and it is handled
by GVN and EarlyCSE.

But anyway, even if IR is not optimal Backend should not crash on a valid IR.

[skatkov]

Fix test by removing dup of CHECK-LABEL.

[dantrushin]

There is method GCStatepointInst::getGCResult() which became invalid now.
You should either remove it or change to return vector (like getGCRelocates()` does) and use it here.
Personally I prefer latter.

[skatkov]

In D98393#2618959, @dantrushin wrote:

There is method GCStatepointInst::getGCResult() which became invalid now.
You should either remove it or change to return vector (like getGCRelocates()` does) and use it here.
Personally I prefer latter.

I removed this method in this patch. No other users of it exists at the moment. In case it is required we can add it.

[dantrushin]

Ah, my bad, I missed that :)
LGTM

llvm/include/llvm/IR/Statepoint.h

public:
 
 
   /// 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
   /// path of invoke.
   inline std::vector<const GCRelocateInst *> getGCRelocates() const;
 
-  /// Get the experimental_gc_result call tied to this statepoint if there is
-  /// one, otherwise return nullptr.
-  const GCResultInst *getGCResult() const {
-    for (auto *U : users())
-      if (auto *GRI = dyn_cast<GCResultInst>(U))
-        return GRI;
-    return nullptr;
-  }
+  /// Returns pair of boolean flags. The first one is true is there is
+  /// a gc.result intrinsic in the same block as statepoint. The second flag
+  /// is true if there is an intrinsic outside of the block with statepoint.
+  inline std::pair<bool, bool> getGCResultLocality() const;
 };
 
 /// Common base class for representing values projected from a statepoint.
 /// Currently, the only projections available are gc.result and gc.relocate.
 class GCProjectionInst : public IntrinsicInst {
 public:
   static bool classof(const IntrinsicInst *I) {
     return I->getIntrinsicID() == Intrinsic::experimental_gc_relocate ||
std::vector<const GCRelocateInst *> GCStatepointInst::getGCRelocates() const {
   // Search for gc relocates that are attached to this landingpad.
   for (const User *LandingPadUser : LandingPad->users()) {
     if (auto *Relocate = dyn_cast<GCRelocateInst>(LandingPadUser))
       Result.push_back(Relocate);
   }
   return Result;
 }
 
+std::pair<bool, bool> GCStatepointInst::getGCResultLocality() const {
+  std::pair<bool, bool> Res(false, false);
+  for (auto *U : users())
+    if (auto *GRI = dyn_cast<GCResultInst>(U)) {
+      if (GRI->getParent() == this->getParent())
+        Res.first = true;
+      else
+        Res.second = true;
+    }
+  return Res;
+}
+
 /// Call sites that get wrapped by a gc.statepoint (currently only in
 /// RewriteStatepointsForGC and potentially in other passes in the future) can
 /// have attributes that describe properties of gc.statepoint call they will be
 /// eventually be wrapped in.  This struct is used represent such directives.
 struct StatepointDirectives {
   Optional<uint32_t> NumPatchBytes;
   Optional<uint64_t> StatepointID;
 

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

#endif
 
   SI.StatepointFlags = I.getFlags();
   SI.NumPatchBytes = I.getNumPatchBytes();
   SI.EHPadBB = EHPadBB;
 
   SDValue ReturnValue = LowerAsSTATEPOINT(SI);
 
   // Export the result value if needed
-  const GCResultInst *GCResult = I.getGCResult();
+  const std::pair<bool, bool> GCResultLocality = I.getGCResultLocality();
   Type *RetTy = I.getActualReturnType();
 
-  if (RetTy->isVoidTy() || !GCResult) {
+  if (RetTy->isVoidTy() ||
+      (!GCResultLocality.first && !GCResultLocality.second)) {
     // The return value is not needed, just generate a poison value. 
     setValue(&I, DAG.getIntPtrConstant(-1, getCurSDLoc()));
     return;
   }
 
-  if (GCResult->getParent() == I.getParent()) {
+  if (GCResultLocality.first) {
     // Result value will be used in a same basic block. Don't export it or
     // perform any explicit register copies. The gc_result will simply grab
     // this value. 
     setValue(&I, ReturnValue);
-    return;
   }
 
+  if (!GCResultLocality.second)
+    return;
   // Result value will be used in a different basic block so we need to export
   // it now.  Default exporting mechanism will not work here because statepoint
   // call has a different type than the actual call. It means that by default
   // llvm will create export register of the wrong type (always i32 in our
   // case). So instead we need to create export register with correct type
   // manually.
   // TODO: To eliminate this problem we can remove gc.result intrinsics
   //       completely and make statepoint call to return a tuple.

llvm/test/CodeGen/X86/statepoint-two-results.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs < %s | FileCheck %s
+
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK-LABEL: quux:
+define void @quux() gc "statepoint-example" {
+; CHECK-LABEL: quux:
+; CHECK:       # %bb.0: # %bb1
+; CHECK-NEXT:    pushq %rax
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    movl $4, %esi
+; CHECK-NEXT:    callq wombat@PLT
+; CHECK-NEXT:  .Ltmp0:
+; CHECK-NEXT:    popq %rax
+; CHECK-NEXT:    .cfi_def_cfa_offset 8
+; CHECK-NEXT:    retq
+bb1:
+  %tmp = call token (i64, i32, i8 addrspace(1)* (i8 addrspace(1)*, i32)*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_p1i8p1i8i32f(i64 2882400000, i32 0, i8 addrspace(1)* (i8 addrspace(1)*, i32)* nonnull @wombat, i32 2, i32 0, i8 addrspace(1)* undef, i32 4, i32 0, i32 0) [ "gc-live"() ]
+  %tmp2 = tail call i8 addrspace(1)* @llvm.experimental.gc.result.p1i8(token %tmp)
+  br label %bb2
+
+bb2:
+  %tmp5 = tail call i8 addrspace(1)* @llvm.experimental.gc.result.p1i8(token %tmp)
+  ret void
+}
+
+declare i8 addrspace(1)* @wombat(i8 addrspace(1)*, i32)
+
+; Function Attrs: nounwind readnone
+declare i8 addrspace(1)* @llvm.experimental.gc.result.p1i8(token) #0
+declare token @llvm.experimental.gc.statepoint.p0f_p1i8p1i8i32f(i64, i32, i8 addrspace(1)* (i8 addrspace(1)*, i32)*, i32, i32, ...)
+attributes #0 = { nounwind readnone }