[MachineSink] Fix an assertion failure in isCycleInvariant

[antmo]

Summary

Fix for #61766. Avoid asserting on dbg_value instr

[sameerds]

llvm/lib/CodeGen/MachineCycleAnalysis.cpp

101–103 ↗

(On Diff #509050)

I am trying to understand how this is relevant to cycle invariance. It seems the intention is that debug instructions cannot be sunk by MachineSink. Then why not do this check there, in the function MachineSinking::FindCycleSinkCandidates()? The check can be done near the same line where isCycleInvariant() is called, ane it will be less confusing.

[antmo]

llvm/lib/CodeGen/MachineCycleAnalysis.cpp

101–103 ↗

(On Diff #509050)

Thanks for the review and comment, sameerds!

This first solution was chosen after reading the comment on line 110 in isCycleInvariant (instr can't e.g. be hoisted, so mark this as not invariant). This also made isCycleInvariant callable on more instructions.

But you're right, it is not clear. I will fix this.
It can also be done by checking isSafeToMove before calling isCycleInvariant in FindCycleSinkCandidates.
This will then stay similar to IsLoopInvariant, called only on IsLICMCandidate (safeToMove) instructions.

[antmo]

moved fix in FindCycleSinkCandidates

[jmorse]

A more generalised solution I believe would be to skip debug instructions while examining the basic block -- that would mean

• Using an iterator-based for loop rather than a range based loop, and
• skipDebugInstructionsForward to increment past any debug instructions at the start of each iteration

Which would mean the rest of the logic in FindCycleSinkCandidates could remain unaltered. (The presence of debug instructions is awkward, but we're slowly trying to get rid of them).

For the test, I would recommend using a MIR test that runs only the machine-sink pass, to minimise the chance of other optimisation passes affecting the coverage of this test.

[antmo]

Thanks for the review and suggestions, jmorse. I will update the patch.

[antmo]

now skipping debug instructions while looping on basic block
replaced ll test by a MIR test

[antmo]

rebase to get test fix

[jmorse]

LGTM -- when landing could you add a short comment to the test indicating which function used to crash, that'll ease the life of anyone reading the test in the future and let them know exactly what's being tested.

llvm/lib/CodeGen/MachineSink.cpp

389–390

You might be able to skip this as I believe we guarantee there are no debug intrinsics after the terminator of a block. On the other hand, it's not going to harm anything either.

[antmo]

updated test

[antmo]

Thanks again for the review @jmorse. I modified the test.

I think the break statement cannot be removed, as it would crash if the last instruction is a debug instruction. Please correct me if I am missing something here.

Also I cannot merge the patch myself. So if everything is OK, could someone do this for me please?

[qcolombet]

> I think the break statement cannot be removed, as it would crash if the last instruction is a debug instruction. Please correct me if I am missing something here.

That's correct. MBB may not have any terminator.

llvm/lib/CodeGen/MachineSink.cpp

 //===- MachineSink.cpp - Sinking for machine instructions -----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 }
 
 void MachineSinking::FindCycleSinkCandidates(
     MachineCycle *Cycle, MachineBasicBlock *BB,
     SmallVectorImpl<MachineInstr *> &Candidates) {
   for (auto &MI : *BB) {
     LLVM_DEBUG(dbgs() << "CycleSink: Analysing candidate: " << MI);
     if (!TII->shouldSink(MI)) {
       LLVM_DEBUG(dbgs() << "CycleSink: Instruction not a candidate for this "
                            "target\n");
       continue;
     }
-    if (!isCycleInvariant(Cycle, MI)) {
-      LLVM_DEBUG(dbgs() << "CycleSink: Instruction is not cycle invariant\n");
-      continue;
-    }
     bool DontMoveAcrossStore = true;
     if (!MI.isSafeToMove(AA, DontMoveAcrossStore)) {
       LLVM_DEBUG(dbgs() << "CycleSink: Instruction not safe to move.\n");
       continue;
     }
+    if (!isCycleInvariant(Cycle, MI)) {
+      LLVM_DEBUG(dbgs() << "CycleSink: Instruction is not cycle invariant\n");
+      continue;
+    }
     if (MI.mayLoad() && !mayLoadFromGOTOrConstantPool(MI)) {
       LLVM_DEBUG(dbgs() << "CycleSink: Dont sink GOT or constant pool loads\n");
       continue;
     }
     if (MI.isConvergent())
       continue;
 
     const MachineOperand &MO = MI.getOperand(0);

llvm/test/CodeGen/Generic/sink-insts-to-avoid-spills-debug-crash.ll

+; RUN: llc -O2 -sink-insts-to-avoid-spills < %s | FileCheck %s
+; REQUIRES: aarch64-registered-target
+
+target triple = "aarch64-unknown-linux-gnu"
+
+; CHECK-LABEL: a:
+define i32 @a() {
+entry:
+  %call = tail call i32 @c()
+  call void @llvm.dbg.value(metadata i32 %call, metadata !3, metadata !DIExpression()), !dbg !8
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.cond, %entry
+  br label %for.cond
+}
+
+declare i32 @c()
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1)
+!1 = !DIFile(filename: "bcm.c", directory: "/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !DILocalVariable(name: "b", scope: !4, file: !1, line: 8, type: !7)
+!4 = distinct !DISubprogram(name: "a", scope: !1, unit: !0)
+!7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!8 = !DILocation(line: 0, scope: !4)