[Greedy RA] Add a check to MachineVerifier

[skatkov]

Summary

If Virtual Register is alive in landing pad its def must be
before the call causing the exception or it should be statepoint instruction itself and
in this case def actually means the relocation of gc pointer and is alive in
landing pad.

The test shows the triggering this check for an option under development
use-registers-for-gc-values-in-landing-pad which is off by default until
it is functionally correct.

[rnk]

Seems reasonable

llvm/lib/CodeGen/MachineVerifier.cpp

2976–2977

I don't see anything statepoint-specific below, so I'm not sure how the second part of this comment is relevant. The first sentence makes sense: EH pads have a special rule that requires virtual registers to be defined before the last call in each predecessor, rather than before the first terminator.

2978–2979

Please use braces when there is an inner compound statement that uses braces. The style guide has an example that supports this interpretation:
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

// Use braces for the outer `if` since the nested `for` is braced.
if (isa<FunctionDecl>(D)) {
  for (auto *A : D.attrs()) {
    // In this for loop body, it is necessary that we explain the situation
    // with this surprisingly long comment, forcing braces on the `for` block.
    handleAttr(A);
  }
}

2981

I believe this is the correct condition.

[skatkov]

@rnk, Reid, thank you very much for the review. Please take a look into updated version.

BTW, I plan to upload a series patches for Greedy RA to fix the crash caused by the adding check, will you be the right person to add you as reviewer for GreedyRA?

[rnk]

lgtm with a nit

llvm/lib/CodeGen/MachineVerifier.cpp

2976

This isEHPad if should use braces as well by the same style rule.

llvm/lib/CodeGen/MachineVerifier.cpp

while (true) {
 
     // Is VNI a PHI-def in the current block?
     bool IsPHI = VNI->isPHIDef() &&
       VNI->def == LiveInts->getMBBStartIdx(&*MFI);
 
     // Check that VNI is live-out of all predecessors.
     for (const MachineBasicBlock *Pred : MFI->predecessors()) {
       SlotIndex PEnd = LiveInts->getMBBEndIdx(Pred);
+      // Predecessor of landing pad live-out on last call.
+      if (MFI->isEHPad()) {
+        for (auto I = Pred->rbegin(), E = Pred->rend(); I != E; ++I) {
+          if (I->isCall()) {
+            PEnd = Indexes->getInstructionIndex(*I).getBoundaryIndex();
+            break;
+          }
+        }
+      }
       const VNInfo *PVNI = LR.getVNInfoBefore(PEnd);
 
       // All predecessors must have a live-out value. However for a phi
       // instruction with subregister intervals
       // only one of the subregisters (not necessarily the current one) needs to
       // be defined.
       if (!PVNI && (LaneMask.none() || !IsPHI)) {
         if (LiveRangeCalc::isJointlyDominated(Pred, Undefs, *Indexes))

llvm/test/CodeGen/X86/statepoint-invoke-ra1.ll

+; REQUIRES: asserts
+; RUN: not  --crash llc -o /dev/null %s -max-registers-for-gc-values=15 -use-registers-for-gc-values-in-landing-pad=true -verify-regalloc 2>&1 | FileCheck %s
+
+; The test checks the verification catch the case when RA splits live interval in the
+; way the def is located after invoke statepoint while use is in landing pad.
+
+; CHECK: *** Bad machine code: Register not marked live out of predecessor ***
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128-ni:1-p2:32:8:8:32-ni:2"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @wombat(i8 addrspace(1)* %arg, i32 %arg1, i32 addrspace(1)* %arg2) gc "statepoint-example" personality i32* ()* @widget {
+bb:
+  %tmp = load i8 addrspace(1)*, i8 addrspace(1)* addrspace(1)* null, align 8
+  %tmp3 = load i32, i32 addrspace(1)* null, align 4
+  %tmp4 = getelementptr inbounds i32, i32 addrspace(1)* %arg2, i64 24
+  %tmp5 = load i32, i32 addrspace(1)* %tmp4, align 4
+  %tmp6 = getelementptr inbounds i32, i32 addrspace(1)* %arg2, i64 40
+  %tmp7 = load i32, i32 addrspace(1)* %tmp6, align 4
+  %tmp8 = load i32, i32 addrspace(1)* null, align 4
+  %tmp9 = load i8 addrspace(1)*, i8 addrspace(1)* addrspace(1)* undef, align 8
+  %tmp10 = getelementptr inbounds i32, i32 addrspace(1)* %arg2, i64 88
+  %tmp11 = load i32, i32 addrspace(1)* %tmp10, align 4
+  %tmp12 = getelementptr inbounds i8, i8 addrspace(1)* %arg, i64 96
+  %tmp13 = bitcast i8 addrspace(1)* %tmp12 to i8 addrspace(1)* addrspace(1)*
+  %tmp14 = load i8 addrspace(1)*, i8 addrspace(1)* addrspace(1)* %tmp13, align 8
+  %tmp15 = getelementptr inbounds i8, i8 addrspace(1)* %arg, i64 104
+  %tmp16 = bitcast i8 addrspace(1)* %tmp15 to i8 addrspace(1)* addrspace(1)*
+  %tmp17 = load i8 addrspace(1)*, i8 addrspace(1)* addrspace(1)* %tmp16, align 8
+  %tmp18 = add i32 %tmp3, -1
+  %tmp19 = load atomic i64, i64 addrspace(1)* undef unordered, align 8
+  %tmp20 = invoke token (i64, i32, i32 (i32, i8 addrspace(1)*, i32, i32, i32)*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_i32i32p1i8i32i32i32f(i64 1, i32 16, i32 (i32, i8 addrspace(1)*, i32, i32, i32)* nonnull @wombat.1, i32 5, i32 0, i32 0, i8 addrspace(1)* null, i32 undef, i32 %arg1, i32 0, i32 0, i32 0) [ "deopt"(i32 %tmp18, i8 addrspace(1)* %tmp, i32 %arg1, i32 %tmp3, i32 %tmp5, i32 %tmp7, i32 %tmp8, i8 addrspace(1)* %tmp9, i32 %tmp11, i8 addrspace(1)* %tmp14, i8 addrspace(1)* %tmp17), "gc-live"(i8 addrspace(1)* %tmp, i8 addrspace(1)* %tmp9, i8 addrspace(1)* %tmp14, i8 addrspace(1)* %tmp17) ]
+          to label %bb21 unwind label %bb26
+
+bb21:                                             ; preds = %bb
+  %tmp22 = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token %tmp20, i32 0, i32 0) ; (%tmp, %tmp)
+  %tmp23 = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token %tmp20, i32 2, i32 2) ; (%tmp14, %tmp14)
+  %tmp24 = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token %tmp20, i32 3, i32 3) ; (%tmp17, %tmp17)
+  %tmp25 = call token (i64, i32, void (i32)*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidi32f(i64 2882400000, i32 0, void (i32)* nonnull @quux, i32 1, i32 2, i32 10, i32 0, i32 0) [ "deopt"(i32 %tmp18, i8 addrspace(1)* %tmp22, i32 %arg1, i32 %tmp3, i32 %tmp5, i32 %tmp7, i32 %tmp8, i32 %tmp11, i8 addrspace(1)* %tmp23, i8 addrspace(1)* %tmp24), "gc-live"() ]
+  ret void
+
+bb26:                                             ; preds = %bb
+  %tmp27 = landingpad token
+          cleanup
+  %tmp28 = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token %tmp27, i32 1, i32 1) ; (%tmp9, %tmp9)
+  %tmp29 = call token (i64, i32, void (i32)*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidi32f(i64 2882400000, i32 0, void (i32)* nonnull @quux, i32 1, i32 0, i32 -271, i32 0, i32 0) [ "deopt"(i32 %arg1, i32 %tmp3, i32 %tmp5, i32 %tmp8, i8 addrspace(1)* %tmp28, i32 %tmp11), "gc-live"() ]
+  unreachable
+}
+
+declare i32* @widget()
+
+declare i32 @wombat.1(i32, i8 addrspace(1)*, i32, i32, i32)
+
+declare void @quux(i32)
+
+declare token @llvm.experimental.gc.statepoint.p0f_isVoidi32f(i64 immarg, i32 immarg, void (i32)*, i32 immarg, i32 immarg, ...)
+
+; Function Attrs: nounwind readonly
+declare i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token, i32 immarg, i32 immarg) #0
+
+declare token @llvm.experimental.gc.statepoint.p0f_i32i32p1i8i32i32i32f(i64 immarg, i32 immarg, i32 (i32, i8 addrspace(1)*, i32, i32, i32)*, i32 immarg, i32 immarg, ...)
+
+attributes #0 = { nounwind readonly }