Hi, The attached patches fix a crash in the AMDILCFGStructurizer. The first patch does some code cleanup and the second patch actually fixes the crash.
-Tom
>From 8a95058178eba343cb6ee408677b42329e7069ed Mon Sep 17 00:00:00 2001 From: Tom Stellard <thomas.stell...@amd.com> Date: Fri, 11 Oct 2013 09:18:22 -0700 Subject: [PATCH 1/2] R600: Remove some dead code from the AMDILCFGStructurizer --- lib/Target/R600/AMDILCFGStructurizer.cpp | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/lib/Target/R600/AMDILCFGStructurizer.cpp b/lib/Target/R600/AMDILCFGStructurizer.cpp index 80190c9..f88d593 100644 --- a/lib/Target/R600/AMDILCFGStructurizer.cpp +++ b/lib/Target/R600/AMDILCFGStructurizer.cpp @@ -1335,32 +1335,11 @@ int AMDGPUCFGStructurizer::improveSimpleJumpintoIf(MachineBasicBlock *HeadMBB, // add initReg = initVal to headBlk const TargetRegisterClass * I32RC = TRI->getCFGStructurizerRegClass(MVT::i32); - unsigned InitReg = - HeadMBB->getParent()->getRegInfo().createVirtualRegister(I32RC); if (!MigrateTrue || !MigrateFalse) llvm_unreachable("Extra register needed to handle CFG"); int NumNewBlk = 0; - if (!LandBlk) { - LandBlk = HeadMBB->getParent()->CreateMachineBasicBlock(); - HeadMBB->getParent()->push_back(LandBlk); //insert to function - - if (TrueMBB) { - TrueMBB->addSuccessor(LandBlk); - } else { - HeadMBB->addSuccessor(LandBlk); - } - - if (FalseMBB) { - FalseMBB->addSuccessor(LandBlk); - } else { - HeadMBB->addSuccessor(LandBlk); - } - - NumNewBlk ++; - } - bool LandBlkHasOtherPred = (LandBlk->pred_size() > 2); //insert AMDGPU::ENDIF to avoid special case "input landBlk == NULL" @@ -1375,6 +1354,10 @@ int AMDGPUCFGStructurizer::improveSimpleJumpintoIf(MachineBasicBlock *HeadMBB, CmpResReg, DebugLoc()); } + // XXX: We are running this after RA, so creating virtual registers will + // cause an assertion failure in the PostRA scheduling pass. + unsigned InitReg = + HeadMBB->getParent()->getRegInfo().createVirtualRegister(I32RC); insertCondBranchBefore(LandBlk, I, AMDGPU::IF_PREDICATE_SET, InitReg, DebugLoc()); -- 1.7.11.4
>From 47c13f2ef5178078313ee9ff93edfc708dda4cc4 Mon Sep 17 00:00:00 2001 From: Tom Stellard <thomas.stell...@amd.com> Date: Fri, 11 Oct 2013 09:18:49 -0700 Subject: [PATCH 2/2] R600: Fix a crash in the AMDILCFGStructurizer We were calling llvm_unreachable() when failing to optimize the branch into if case. However, it is still possible for us to structurize the CFG by duplicating blocks even if this optimization fails. --- lib/Target/R600/AMDILCFGStructurizer.cpp | 70 ++++++++++++++++++++++++++- test/CodeGen/R600/structurize.ll | 83 ++++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 test/CodeGen/R600/structurize.ll diff --git a/lib/Target/R600/AMDILCFGStructurizer.cpp b/lib/Target/R600/AMDILCFGStructurizer.cpp index f88d593..a9337f0 100644 --- a/lib/Target/R600/AMDILCFGStructurizer.cpp +++ b/lib/Target/R600/AMDILCFGStructurizer.cpp @@ -1335,8 +1335,74 @@ int AMDGPUCFGStructurizer::improveSimpleJumpintoIf(MachineBasicBlock *HeadMBB, // add initReg = initVal to headBlk const TargetRegisterClass * I32RC = TRI->getCFGStructurizerRegClass(MVT::i32); - if (!MigrateTrue || !MigrateFalse) - llvm_unreachable("Extra register needed to handle CFG"); + if (!MigrateTrue || !MigrateFalse) { + // XXX: We have an opportunity here to optimize the "branch into if" case + // here. Branch into if looks like this: + // entry + // / \ + // diamond_head branch_from + // / \ | + // diamond_false diamond_true + // \ / + // done + // + // The diamond_head block begins the "if" and the diamond_true block + // is the block being "branched into". + // + // If MigrateTrue is true, then TrueBB is the block being "branched into" + // and if MigrateFalse is true, then FalseBB is the block being + // "branched into" + // + // Here is the pseudo code for how I think the optimization should work: + // 1. Insert MOV GPR0, 0 before the branch instruction in diamond_head. + // 2. Insert MOV GPR0, 1 before the branch instruction in branch_from. + // 3. Move the branch instruction from diamond_head into its own basic + // block (new_block). + // 4. Add an unconditional branch from diamond_head to new_block + // 5. Replace the branch instruction in branch_from with an unconditional + // branch to new_block. If branch_from has multiple predecessors, then + // we need to replace the True/False block in the branch + // instruction instead of replacing it. + // 6. Change the condition of the branch instruction in new_block from + // COND to (COND || GPR0) + // + // In order insert these MOV instruction, we will need to use the + // RegisterScavenger. Usually liveness stops being tracked during + // the late machine optimization passes, however if we implement + // bool TargetRegisterInfo::requiresRegisterScavenging( + // const MachineFunction &MF) + // and have it return true, liveness will be tracked correctly + // by generic optimization passes. We will also need to make sure that + // all of our target-specific passes that run after regalloc and before + // the CFGStructurizer track liveness and we will need to modify this pass + // to correctly track liveness. + // + // After the above changes, the new CFG should look like this: + // entry + // / \ + // diamond_head branch_from + // \ / + // new_block + // / \ + // diamond_false diamond_true + // \ / + // done + // + // Without this optimization, we are forced to duplicate the diamond_true + // block and we will end up with a CFG like this: + // + // entry + // / \ + // diamond_head branch_from + // / \ | + // diamond_false diamond_true diamond_true (duplicate) + // \ / | + // done --------------------| + // + // Duplicating diamond_true can be very costly especially if it has a + // lot of instructions. + return 0; + } int NumNewBlk = 0; diff --git a/test/CodeGen/R600/structurize.ll b/test/CodeGen/R600/structurize.ll new file mode 100644 index 0000000..b955619 --- /dev/null +++ b/test/CodeGen/R600/structurize.ll @@ -0,0 +1,83 @@ +; RUN: llc < %s -march=r600 -mcpu=redwood | FileCheck %s +; Test case for a crash in the AMDILCFGStructurizer from a CFG like this: +; +; entry +; / \ +; diamond_head branch_from +; / \ | +; diamond_false diamond_true +; \ / +; done +; +; When the diamond_true branch had more than 100 instructions. +; +; + +; CHECK-LABEL: @branch_into_diamond +; === entry block: +; CHECK: ALU_PUSH_BEFORE +; === Branch instruction (IF): +; CHECK: JUMP + ; === branch_from block + ; CHECK: ALU + ; === Duplicated diamond_true block (There can be more than one ALU clause): + ; === XXX: We should be able to optimize this so the basic block is not + ; === duplicated. See comments in + ; === AMDGPUCFGStructurizer::improveSimpleJumpintoIf() + ; CHECK: ALU +; === Branch instruction (ELSE): +; CHECK: ELSE + ; === diamond_head block: + ; CHECK: ALU_PUSH_BEFORE + ; === Branch instruction (IF): + ; CHECK: JUMP + ; === diamond_true block (There can be more than one ALU clause): + ; ALU + ; === Branch instruction (ELSE): + ; CHECK: ELSE + ; === diamond_false block plus implicit ENDIF + ; CHECK: ALU_POP_AFTER +; === Branch instruction (ENDIF): +; CHECK: POP +; === done block: +; CHECK: ALU +; CHECK: MEM_RAT_CACHELESS +; CHECK: CF_END + + +define void @branch_into_diamond(i32 addrspace(1)* %out, i32 %a, i32 %b, i32 %c) { +entry: +%0 = icmp ne i32 %a, 0 + br i1 %0, label %diamond_head, label %branch_from + +diamond_head: + %1 = icmp ne i32 %a, 1 + br i1 %1, label %diamond_true, label %diamond_false + +branch_from: + %2 = add i32 %a, 1 + br label %diamond_true + +diamond_false: + %3 = add i32 %a, 2 + br label %done + +diamond_true: + %4 = phi i32 [%2, %branch_from], [%a, %diamond_head] + ; This block needs to be > 100 ISA instructions to hit the bug, + ; so we'll use udiv instructions. + %div0 = udiv i32 %a, %b + %div1 = udiv i32 %div0, %4 + %div2 = udiv i32 %div1, 11 + %div3 = udiv i32 %div2, %a + %div4 = udiv i32 %div3, %b + %div5 = udiv i32 %div4, %c + %div6 = udiv i32 %div5, %div0 + %div7 = udiv i32 %div6, %div1 + br label %done + +done: + %5 = phi i32 [%3, %diamond_false], [%div7, %diamond_true] + store i32 %5, i32 addrspace(1)* %out + ret void +} -- 1.7.11.4
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev