From 2a9f1dad2cceb70be372310e46eaf93f32782a43 Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Sun, 5 Mar 2023 07:07:23 -0400 Subject: [PATCH] AMDGPU: Fix LiveVariables verifier error for values defined before SI_END_CF GlobalISel happens to insert some constant materializes before SI_END_CF in one test. These need to be excluded from AliveBlocks since they are defined in the original block and used in the split block, so they aren't fully alive through either block. The case where the value defined in the first block which was originally used in a later block is still broken. Avoids a verifier error in a future patch. --- llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp | 21 +++-- ...wer-control-flow-live-variables-update.mir | 82 +++++++++++++++++++ ...ntrol-flow-live-variables-update.xfail.mir | 42 ++++++++++ 3 files changed, 137 insertions(+), 8 deletions(-) create mode 100644 llvm/test/CodeGen/AMDGPU/lower-control-flow-live-variables-update.xfail.mir diff --git a/llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp b/llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp index 67077a2eaa6b..0e2bf9dad64f 100644 --- a/llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp +++ b/llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp @@ -514,13 +514,18 @@ MachineBasicBlock *SILowerControlFlow::emitEndCf(MachineInstr &MI) { LV->replaceKillInstruction(DataReg, MI, *NewMI); if (SplitBB != &MBB) { - // Track the set of registers defined in the split block so we don't - // accidentally add the original block to AliveBlocks. - DenseSet SplitDefs; - for (MachineInstr &X : *SplitBB) { - for (MachineOperand &Op : X.operands()) { - if (Op.isReg() && Op.isDef() && Op.getReg().isVirtual()) - SplitDefs.insert(Op.getReg()); + // Track the set of registers defined in the original block so we don't + // accidentally add the original block to AliveBlocks. AliveBlocks only + // includes blocks which are live through, which excludes live outs and + // local defs. + DenseSet DefInOrigBlock; + + for (MachineBasicBlock *BlockPiece : {&MBB, SplitBB}) { + for (MachineInstr &X : *BlockPiece) { + for (MachineOperand &Op : X.operands()) { + if (Op.isReg() && Op.isDef() && Op.getReg().isVirtual()) + DefInOrigBlock.insert(Op.getReg()); + } } } @@ -532,7 +537,7 @@ MachineBasicBlock *SILowerControlFlow::emitEndCf(MachineInstr &MI) { VI.AliveBlocks.set(SplitBB->getNumber()); else { for (MachineInstr *Kill : VI.Kills) { - if (Kill->getParent() == SplitBB && !SplitDefs.contains(Reg)) + if (Kill->getParent() == SplitBB && !DefInOrigBlock.contains(Reg)) VI.AliveBlocks.set(MBB.getNumber()); } } diff --git a/llvm/test/CodeGen/AMDGPU/lower-control-flow-live-variables-update.mir b/llvm/test/CodeGen/AMDGPU/lower-control-flow-live-variables-update.mir index 6a2742a772bf..f04f66cfbba1 100644 --- a/llvm/test/CodeGen/AMDGPU/lower-control-flow-live-variables-update.mir +++ b/llvm/test/CodeGen/AMDGPU/lower-control-flow-live-variables-update.mir @@ -178,3 +178,85 @@ body: | S_BRANCH %bb.3 ... + +# Check we don't get "Block should not be in AliveBlocks" for +# registers defined before si_end_cf +--- +name: live_variables_update_block_split_split_killed_def_before_si_end_cf +tracksRegLiveness: true +body: | + ; CHECK-LABEL: name: live_variables_update_block_split_split_killed_def_before_si_end_cf + ; CHECK: bb.0: + ; CHECK-NEXT: successors: %bb.1(0x40000000), %bb.2(0x40000000) + ; CHECK-NEXT: liveins: $vgpr0 + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: [[COPY:%[0-9]+]]:vgpr_32 = COPY killed $vgpr0 + ; CHECK-NEXT: [[V_MOV_B32_e32_:%[0-9]+]]:vgpr_32 = V_MOV_B32_e32 0, implicit $exec + ; CHECK-NEXT: [[V_CMP_EQ_U32_e64_:%[0-9]+]]:sreg_64_xexec = V_CMP_EQ_U32_e64 0, killed [[COPY]], implicit $exec + ; CHECK-NEXT: [[COPY1:%[0-9]+]]:vgpr_32 = COPY [[V_MOV_B32_e32_]] + ; CHECK-NEXT: [[COPY2:%[0-9]+]]:vgpr_32 = COPY killed [[V_MOV_B32_e32_]] + ; CHECK-NEXT: [[COPY3:%[0-9]+]]:sreg_64 = COPY $exec, implicit-def $exec + ; CHECK-NEXT: [[S_AND_B64_:%[0-9]+]]:sreg_64 = S_AND_B64 [[COPY3]], [[V_CMP_EQ_U32_e64_]], implicit-def dead $scc + ; CHECK-NEXT: [[S_XOR_B64_:%[0-9]+]]:sreg_64_xexec = S_XOR_B64 [[S_AND_B64_]], [[COPY3]], implicit-def dead $scc + ; CHECK-NEXT: $exec = S_MOV_B64_term killed [[S_AND_B64_]] + ; CHECK-NEXT: [[S_MOV_B64_term:%[0-9]+]]:sreg_64_xexec = S_MOV_B64_term killed [[S_XOR_B64_]], implicit $exec + ; CHECK-NEXT: S_CBRANCH_EXECZ %bb.1, implicit $exec + ; CHECK-NEXT: S_BRANCH %bb.2 + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: bb.1: + ; CHECK-NEXT: successors: %bb.3(0x80000000) + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: [[COPY4:%[0-9]+]]:sreg_64_xexec = COPY killed [[S_MOV_B64_term]] + ; CHECK-NEXT: [[S_MOV_B64_:%[0-9]+]]:sreg_64 = S_MOV_B64 1 + ; CHECK-NEXT: $exec = S_OR_B64_term $exec, killed [[COPY4]], implicit-def $scc + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: bb.3: + ; CHECK-NEXT: successors: %bb.2(0x80000000) + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: S_NOP 0, implicit killed [[S_MOV_B64_]] + ; CHECK-NEXT: [[COPY5:%[0-9]+]]:vgpr_32 = COPY killed [[COPY1]] + ; CHECK-NEXT: [[V_ADD_U32_e32_:%[0-9]+]]:vgpr_32 = nsw V_ADD_U32_e32 1, killed [[COPY5]], implicit $exec + ; CHECK-NEXT: [[COPY6:%[0-9]+]]:vgpr_32 = COPY killed [[V_ADD_U32_e32_]] + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: bb.2: + ; CHECK-NEXT: successors: %bb.2(0x40000000), %bb.1(0x40000000) + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: [[COPY66:%[0-9]+]]:vgpr_32 = COPY killed [[COPY6]] + ; CHECK-NEXT: GLOBAL_STORE_DWORD undef %11:vreg_64, [[COPY66]], 0, 0, implicit $exec :: (volatile store (s32), addrspace 1) + ; CHECK-NEXT: [[COPY7:%[0-9]+]]:vgpr_32 = COPY killed [[COPY66]] + ; CHECK-NEXT: [[COPY8:%[0-9]+]]:vgpr_32 = COPY [[COPY7]] + ; CHECK-NEXT: [[COPY8:%[0-9]+]]:vgpr_32 = COPY killed [[COPY7]] + ; CHECK-NEXT: [[COPY8:%[0-9]+]]:sreg_64 = COPY $exec, implicit-def $exec + ; CHECK-NEXT: [[S_AND_B64_1:%[0-9]+]]:sreg_64 = S_AND_B64 [[COPY8]], [[V_CMP_EQ_U32_e64_]], implicit-def dead $scc + ; CHECK-NEXT: [[S_XOR_B64_1:%[0-9]+]]:sreg_64_xexec = S_XOR_B64 [[S_AND_B64_1]], [[COPY8]], implicit-def dead $scc + ; CHECK-NEXT: $exec = S_MOV_B64_term killed [[S_AND_B64_1]] + ; CHECK-NEXT: [[S_MOV_B64_term1:%[0-9]+]]:sreg_64_xexec = S_MOV_B64_term killed [[S_XOR_B64_1]], implicit $exec + ; CHECK-NEXT: S_CBRANCH_EXECZ %bb.1, implicit $exec + ; CHECK-NEXT: S_BRANCH %bb.2 + bb.0: + liveins: $vgpr0 + + %0:vgpr_32 = COPY killed $vgpr0 + %1:vgpr_32 = V_MOV_B32_e32 0, implicit $exec + %2:sreg_64_xexec = V_CMP_EQ_U32_e64 0, killed %0, implicit $exec + %3:sreg_64_xexec = SI_IF %2, %bb.1, implicit-def $exec, implicit-def dead $scc, implicit $exec + S_BRANCH %bb.2 + + bb.1: + %4:sreg_64_xexec = PHI %5, %bb.2, %3, %bb.0 + %6:vgpr_32 = PHI %7, %bb.2, %1, %bb.0 + %8:sreg_64 = S_MOV_B64 1 + SI_END_CF killed %4, implicit-def $exec, implicit-def dead $scc, implicit $exec + S_NOP 0, implicit killed %8 + %9:vgpr_32 = nsw V_ADD_U32_e32 1, killed %6, implicit $exec + + bb.2: + successors: %bb.2(0x40000000), %bb.1(0x40000000) + + %10:vgpr_32 = PHI %9, %bb.1, %7, %bb.2, %1, %bb.0 + GLOBAL_STORE_DWORD undef %11:vreg_64, %10, 0, 0, implicit $exec :: (volatile store (s32), addrspace 1) + %7:vgpr_32 = COPY killed %10 + %5:sreg_64_xexec = SI_IF %2, %bb.1, implicit-def $exec, implicit-def dead $scc, implicit $exec + S_BRANCH %bb.2 + +... diff --git a/llvm/test/CodeGen/AMDGPU/lower-control-flow-live-variables-update.xfail.mir b/llvm/test/CodeGen/AMDGPU/lower-control-flow-live-variables-update.xfail.mir new file mode 100644 index 000000000000..f4e26aeae676 --- /dev/null +++ b/llvm/test/CodeGen/AMDGPU/lower-control-flow-live-variables-update.xfail.mir @@ -0,0 +1,42 @@ +# RUN: not --crash llc -mtriple=amdgcn-amd-amdhsa -start-before=livevars -stop-after=twoaddressinstruction -verify-machineinstrs -o - %s 2>&1 | FileCheck %s + +# CHECK: *** Bad machine code: LiveVariables: Block missing from AliveBlocks *** +# CHECK-NEXT: function: live_variables_update_block_split_split_def_before_si_end_cf_live_out +# CHECK-NEXT: basic block: %bb.4 +# CHECK-NEXT: Virtual register %8 must be live through the block. + + +# Same as +# live_variables_update_block_split_split_killed_def_before_si_end_cf, +# except the def before si_end_cf is live out of the block +--- +name: live_variables_update_block_split_split_def_before_si_end_cf_live_out +tracksRegLiveness: true +body: | + bb.0: + liveins: $vgpr0 + + %0:vgpr_32 = COPY killed $vgpr0 + %1:vgpr_32 = V_MOV_B32_e32 0, implicit $exec + %2:sreg_64_xexec = V_CMP_EQ_U32_e64 0, killed %0, implicit $exec + %3:sreg_64_xexec = SI_IF %2, %bb.1, implicit-def $exec, implicit-def dead $scc, implicit $exec + S_BRANCH %bb.3 + + bb.1: + %4:sreg_64_xexec = PHI %5, %bb.3, %3, %bb.0 + %6:vgpr_32 = PHI %7, %bb.3, %1, %bb.0 + %8:sreg_64 = S_MOV_B64 1 + SI_END_CF killed %4, implicit-def $exec, implicit-def dead $scc, implicit $exec + %9:vgpr_32 = nsw V_ADD_U32_e32 1, killed %6, implicit $exec + + bb.2: + S_NOP 0, implicit killed %8 + + bb.3: + %10:vgpr_32 = PHI %9, %bb.2, %7, %bb.3, %1, %bb.0 + GLOBAL_STORE_DWORD undef %11:vreg_64, %10, 0, 0, implicit $exec :: (volatile store (s32), addrspace 1) + %7:vgpr_32 = COPY killed %10 + %5:sreg_64_xexec = SI_IF %2, %bb.1, implicit-def $exec, implicit-def dead $scc, implicit $exec + S_BRANCH %bb.3 + +...