From 25ed97481416e7353e425bbcfe3ab321662b5f03 Mon Sep 17 00:00:00 2001 From: Benjamin Kramer Date: Wed, 27 Jan 2016 12:44:12 +0000 Subject: [PATCH] Revert "Allow X86::COND_NE_OR_P and X86::COND_NP_OR_E to be reversed." and "Add a missing test case for r258847." This reverts commit r258847, r258848. Causes miscompilations and backend errors. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@258927 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Target/X86/X86InstrInfo.cpp | 94 ++++--------------- lib/Target/X86/X86InstrInfo.h | 63 ++++++------- test/CodeGen/X86/block-placement.ll | 27 +++--- test/CodeGen/X86/fast-isel-cmp-branch2.ll | 5 +- test/CodeGen/X86/fast-isel-cmp-branch3.ll | 5 +- test/CodeGen/X86/fp-une-cmp.ll | 33 +------ test/CodeGen/X86/x86-analyze-branch-jne-jp.ll | 21 ----- 7 files changed, 68 insertions(+), 180 deletions(-) delete mode 100644 test/CodeGen/X86/x86-analyze-branch-jne-jp.ll diff --git a/lib/Target/X86/X86InstrInfo.cpp b/lib/Target/X86/X86InstrInfo.cpp index f0ae57e2518..0c7ce7c2490 100644 --- a/lib/Target/X86/X86InstrInfo.cpp +++ b/lib/Target/X86/X86InstrInfo.cpp @@ -3805,10 +3805,6 @@ X86::CondCode X86::GetOppositeBranchCondition(X86::CondCode CC) { case X86::COND_NP: return X86::COND_P; case X86::COND_O: return X86::COND_NO; case X86::COND_NO: return X86::COND_O; - case X86::COND_NE_OR_P: return X86::COND_E_AND_NP; - case X86::COND_NP_OR_E: return X86::COND_P_AND_NE; - case X86::COND_E_AND_NP: return X86::COND_NE_OR_P; - case X86::COND_P_AND_NE: return X86::COND_NP_OR_E; } } @@ -4002,9 +3998,9 @@ bool X86InstrInfo::AnalyzeBranchImpl( MachineBasicBlock::iterator OldInst = I; BuildMI(MBB, UnCondBrIter, MBB.findDebugLoc(I), get(JNCC)) - .addMBB(UnCondBrIter->getOperand(0).getMBB()); + .addMBB(UnCondBrIter->getOperand(0).getMBB()); BuildMI(MBB, UnCondBrIter, MBB.findDebugLoc(I), get(X86::JMP_1)) - .addMBB(TargetBB); + .addMBB(TargetBB); OldInst->eraseFromParent(); UnCondBrIter->eraseFromParent(); @@ -4028,6 +4024,11 @@ bool X86InstrInfo::AnalyzeBranchImpl( assert(Cond.size() == 1); assert(TBB); + // Only handle the case where all conditional branches branch to the same + // destination. + if (TBB != I->getOperand(0).getMBB()) + return true; + // If the conditions are the same, we can leave them alone. X86::CondCode OldBranchCode = (X86::CondCode)Cond[0].getImm(); if (OldBranchCode == BranchCode) @@ -4036,40 +4037,17 @@ bool X86InstrInfo::AnalyzeBranchImpl( // If they differ, see if they fit one of the known patterns. Theoretically, // we could handle more patterns here, but we shouldn't expect to see them // if instruction selection has done a reasonable job. - auto NewTBB = I->getOperand(0).getMBB(); - if (TBB == NewTBB && - ((OldBranchCode == X86::COND_NP && BranchCode == X86::COND_E) || - (OldBranchCode == X86::COND_E && BranchCode == X86::COND_NP))) { + if ((OldBranchCode == X86::COND_NP && + BranchCode == X86::COND_E) || + (OldBranchCode == X86::COND_E && + BranchCode == X86::COND_NP)) BranchCode = X86::COND_NP_OR_E; - } else if (TBB == NewTBB && - ((OldBranchCode == X86::COND_P && BranchCode == X86::COND_NE) || - (OldBranchCode == X86::COND_NE && BranchCode == X86::COND_P))) { + else if ((OldBranchCode == X86::COND_P && + BranchCode == X86::COND_NE) || + (OldBranchCode == X86::COND_NE && + BranchCode == X86::COND_P)) BranchCode = X86::COND_NE_OR_P; - } else if ((OldBranchCode == X86::COND_NE && BranchCode == X86::COND_NP) || - (OldBranchCode == X86::COND_P && BranchCode == X86::COND_E)) { - // X86::COND_P_AND_NE usually has two different branch destinations. - // - // JNP B1 - // JNE B2 - // B1: (fall-through) - // B2: - // - // Here this condition branches to B2 only if P && NE. It has another - // equivalent form: - // - // JE B1 - // JP B2 - // B1: (fall-through) - // B2: - // - // Similarly it branches to B2 only if NE && P. That is why this condition - // is named COND_P_AND_NE. - BranchCode = X86::COND_P_AND_NE; - } else if ((OldBranchCode == X86::COND_NP && BranchCode == X86::COND_NE) || - (OldBranchCode == X86::COND_E && BranchCode == X86::COND_P)) { - // See comments above for X86::COND_P_AND_NE. - BranchCode = X86::COND_E_AND_NP; - } else + else return true; // Update the MachineOperand. @@ -4178,13 +4156,6 @@ unsigned X86InstrInfo::RemoveBranch(MachineBasicBlock &MBB) const { return Count; } -static MachineBasicBlock *getFallThroughMBB(MachineBasicBlock *MBB) { - auto I = std::next(MBB->getIterator()); - if (I == MBB->getParent()->end()) - return nullptr; - return &*I; -} - unsigned X86InstrInfo::InsertBranch(MachineBasicBlock &MBB, MachineBasicBlock *TBB, MachineBasicBlock *FBB, ArrayRef Cond, @@ -4201,9 +4172,6 @@ X86InstrInfo::InsertBranch(MachineBasicBlock &MBB, MachineBasicBlock *TBB, return 1; } - // If FBB is null, it is implied to be a fall-through block. - bool FallThru = FBB == nullptr; - // Conditional branch. unsigned Count = 0; X86::CondCode CC = (X86::CondCode)Cond[0].getImm(); @@ -4222,39 +4190,13 @@ X86InstrInfo::InsertBranch(MachineBasicBlock &MBB, MachineBasicBlock *TBB, BuildMI(&MBB, DL, get(X86::JP_1)).addMBB(TBB); ++Count; break; - case X86::COND_P_AND_NE: - // Use the next block of MBB as FBB if it is null. - if (FBB == nullptr) { - FBB = getFallThroughMBB(&MBB); - assert(FBB && "MBB cannot be the last block in function when the false " - "body is a fall-through."); - } - // Synthesize NEG_NP_OR_E with two branches. - BuildMI(&MBB, DL, get(X86::JNP_1)).addMBB(FBB); - ++Count; - BuildMI(&MBB, DL, get(X86::JNE_1)).addMBB(TBB); - ++Count; - break; - case X86::COND_E_AND_NP: - // Use the next block of MBB as FBB if it is null. - if (FBB == nullptr) { - FBB = getFallThroughMBB(&MBB); - assert(FBB && "MBB cannot be the last block in function when the false " - "body is a fall-through."); - } - // Synthesize NEG_NE_OR_P with two branches. - BuildMI(&MBB, DL, get(X86::JNE_1)).addMBB(FBB); - ++Count; - BuildMI(&MBB, DL, get(X86::JNP_1)).addMBB(TBB); - ++Count; - break; default: { unsigned Opc = GetCondBranchFromCond(CC); BuildMI(&MBB, DL, get(Opc)).addMBB(TBB); ++Count; } } - if (!FallThru) { + if (FBB) { // Two-way Conditional branch. Insert the second branch. BuildMI(&MBB, DL, get(X86::JMP_1)).addMBB(FBB); ++Count; @@ -6775,6 +6717,8 @@ bool X86InstrInfo:: ReverseBranchCondition(SmallVectorImpl &Cond) const { assert(Cond.size() == 1 && "Invalid X86 branch condition!"); X86::CondCode CC = static_cast(Cond[0].getImm()); + if (CC == X86::COND_NE_OR_P || CC == X86::COND_NP_OR_E) + return true; Cond[0].setImm(GetOppositeBranchCondition(CC)); return false; } diff --git a/lib/Target/X86/X86InstrInfo.h b/lib/Target/X86/X86InstrInfo.h index 10d178f7486..edd09d61759 100644 --- a/lib/Target/X86/X86InstrInfo.h +++ b/lib/Target/X86/X86InstrInfo.h @@ -29,44 +29,35 @@ namespace llvm { namespace X86 { // X86 specific condition code. These correspond to X86_*_COND in // X86InstrInfo.td. They must be kept in synch. -enum CondCode { - COND_A = 0, - COND_AE = 1, - COND_B = 2, - COND_BE = 3, - COND_E = 4, - COND_G = 5, - COND_GE = 6, - COND_L = 7, - COND_LE = 8, - COND_NE = 9, - COND_NO = 10, - COND_NP = 11, - COND_NS = 12, - COND_O = 13, - COND_P = 14, - COND_S = 15, - LAST_VALID_COND = COND_S, + enum CondCode { + COND_A = 0, + COND_AE = 1, + COND_B = 2, + COND_BE = 3, + COND_E = 4, + COND_G = 5, + COND_GE = 6, + COND_L = 7, + COND_LE = 8, + COND_NE = 9, + COND_NO = 10, + COND_NP = 11, + COND_NS = 12, + COND_O = 13, + COND_P = 14, + COND_S = 15, + LAST_VALID_COND = COND_S, - // Artificial condition codes. These are used by AnalyzeBranch - // to indicate a block terminated with two conditional branches to - // the same location. This occurs in code using FCMP_OEQ or FCMP_UNE, - // which can't be represented on x86 with a single condition. These - // are never used in MachineInstrs. - COND_NE_OR_P, - COND_NP_OR_E, + // Artificial condition codes. These are used by AnalyzeBranch + // to indicate a block terminated with two conditional branches to + // the same location. This occurs in code using FCMP_OEQ or FCMP_UNE, + // which can't be represented on x86 with a single condition. These + // are never used in MachineInstrs. + COND_NE_OR_P, + COND_NP_OR_E, - // Artificial condition codes. These are used to represent the negation of - // above two conditions. The only scenario we need these two conditions is - // when we try to reverse above two conditions in order to remove redundant - // unconditional jumps. Note that both true and false bodies need to be - // avaiable in order to correctly synthesize instructions for them. These are - // never used in MachineInstrs. - COND_E_AND_NP, // negate of COND_NE_OR_P - COND_P_AND_NE, // negate of COND_NP_OR_E - - COND_INVALID -}; + COND_INVALID + }; // Turn condition code into conditional branch opcode. unsigned GetCondBranchFromCond(CondCode CC); diff --git a/test/CodeGen/X86/block-placement.ll b/test/CodeGen/X86/block-placement.ll index 2b16e417385..98d37153876 100644 --- a/test/CodeGen/X86/block-placement.ll +++ b/test/CodeGen/X86/block-placement.ll @@ -463,23 +463,26 @@ exit: } define void @fpcmp_unanalyzable_branch(i1 %cond) { -; This function's CFG contains an once-unanalyzable branch (une on floating -; points). As now it becomes analyzable, we should get best layout in which each -; edge in 'entry' -> 'entry.if.then_crit_edge' -> 'if.then' -> 'if.end' is -; fall-through. +; This function's CFG contains an unanalyzable branch that is likely to be +; split due to having a different high-probability predecessor. ; CHECK: fpcmp_unanalyzable_branch ; CHECK: %entry -; CHECK: %entry.if.then_crit_edge -; CHECK: %if.then -; CHECK: %if.end ; CHECK: %exit +; CHECK-NOT: %if.then +; CHECK-NOT: %if.end +; CHECK-NOT: jne +; CHECK-NOT: jnp ; CHECK: jne ; CHECK-NEXT: jnp +; CHECK-NEXT: %if.then entry: ; Note that this branch must be strongly biased toward ; 'entry.if.then_crit_edge' to ensure that we would try to form a chain for -; 'entry' -> 'entry.if.then_crit_edge' -> 'if.then' -> 'if.end'. +; 'entry' -> 'entry.if.then_crit_edge' -> 'if.then'. It is the last edge in that +; chain which would violate the unanalyzable branch in 'exit', but we won't even +; try this trick unless 'if.then' is believed to almost always be reached from +; 'entry.if.then_crit_edge'. br i1 %cond, label %entry.if.then_crit_edge, label %lor.lhs.false, !prof !1 entry.if.then_crit_edge: @@ -491,7 +494,7 @@ lor.lhs.false: exit: %cmp.i = fcmp une double 0.000000e+00, undef - br i1 %cmp.i, label %if.then, label %if.end, !prof !3 + br i1 %cmp.i, label %if.then, label %if.end if.then: %0 = phi i8 [ %.pre14, %entry.if.then_crit_edge ], [ undef, %exit ] @@ -504,7 +507,6 @@ if.end: } !1 = !{!"branch_weights", i32 1000, i32 1} -!3 = !{!"branch_weights", i32 1, i32 1000} declare i32 @f() declare i32 @g() @@ -663,14 +665,11 @@ define void @unanalyzable_branch_to_best_succ(i1 %cond) { ; Ensure that we can handle unanalyzable branches where the destination block ; gets selected as the optimal successor to merge. ; -; This branch is now analyzable and hence the destination block becomes the -; hotter one. The right order is entry->bar->exit->foo. -; ; CHECK: unanalyzable_branch_to_best_succ ; CHECK: %entry +; CHECK: %foo ; CHECK: %bar ; CHECK: %exit -; CHECK: %foo entry: ; Bias this branch toward bar to ensure we form that chain. diff --git a/test/CodeGen/X86/fast-isel-cmp-branch2.ll b/test/CodeGen/X86/fast-isel-cmp-branch2.ll index 475d8fcf7f3..04dbac07690 100644 --- a/test/CodeGen/X86/fast-isel-cmp-branch2.ll +++ b/test/CodeGen/X86/fast-isel-cmp-branch2.ll @@ -5,7 +5,7 @@ define i32 @fcmp_oeq(float %x, float %y) { ; CHECK-LABEL: fcmp_oeq ; CHECK: ucomiss %xmm1, %xmm0 ; CHECK-NEXT: jne {{LBB.+_1}} -; CHECK-NEXT: jp {{LBB.+_1}} +; CHECK-NEXT: jnp {{LBB.+_2}} %1 = fcmp oeq float %x, %y br i1 %1, label %bb1, label %bb2 bb2: @@ -162,7 +162,8 @@ define i32 @fcmp_une(float %x, float %y) { ; CHECK-LABEL: fcmp_une ; CHECK: ucomiss %xmm1, %xmm0 ; CHECK-NEXT: jne {{LBB.+_2}} -; CHECK-NEXT: jnp {{LBB.+_1}} +; CHECK-NEXT: jp {{LBB.+_2}} +; CHECK-NEXT: jmp {{LBB.+_1}} %1 = fcmp une float %x, %y br i1 %1, label %bb1, label %bb2 bb2: diff --git a/test/CodeGen/X86/fast-isel-cmp-branch3.ll b/test/CodeGen/X86/fast-isel-cmp-branch3.ll index 8f09b2e3835..e54d0ca4007 100644 --- a/test/CodeGen/X86/fast-isel-cmp-branch3.ll +++ b/test/CodeGen/X86/fast-isel-cmp-branch3.ll @@ -17,7 +17,7 @@ define i32 @fcmp_oeq2(float %x) { ; CHECK: xorps %xmm1, %xmm1 ; CHECK-NEXT: ucomiss %xmm1, %xmm0 ; CHECK-NEXT: jne {{LBB.+_1}} -; CHECK-NEXT: jp {{LBB.+_1}} +; CHECK-NEXT: jnp {{LBB.+_2}} %1 = fcmp oeq float %x, 0.000000e+00 br i1 %1, label %bb1, label %bb2 bb2: @@ -338,7 +338,8 @@ define i32 @fcmp_une2(float %x) { ; CHECK: xorps %xmm1, %xmm1 ; CHECK-NEXT: ucomiss %xmm1, %xmm0 ; CHECK-NEXT: jne {{LBB.+_2}} -; CHECK-NEXT: jnp {{LBB.+_1}} +; CHECK-NEXT: jp {{LBB.+_2}} +; CHECK-NEXT: jmp {{LBB.+_1}} %1 = fcmp une float %x, 0.000000e+00 br i1 %1, label %bb1, label %bb2 bb2: diff --git a/test/CodeGen/X86/fp-une-cmp.ll b/test/CodeGen/X86/fp-une-cmp.ll index 712ce4165b4..7f772d11da9 100644 --- a/test/CodeGen/X86/fp-une-cmp.ll +++ b/test/CodeGen/X86/fp-une-cmp.ll @@ -19,12 +19,12 @@ ; addsd ... ; LBB0_2: -define float @func1(float %x, float %y) nounwind readnone optsize ssp { -; CHECK: func1 +; CHECK: func ; CHECK: jne [[LABEL:.*]] ; CHECK-NEXT: jp [[LABEL]] ; CHECK-NOT: jmp -; + +define float @func(float %x, float %y) nounwind readnone optsize ssp { entry: %0 = fpext float %x to double %1 = fpext float %y to double @@ -41,30 +41,3 @@ bb2: %.0 = fptrunc double %.0.in to float ret float %.0 } - -define float @func2(float %x, float %y) nounwind readnone optsize ssp { -; CHECK: func2 -; CHECK: jne [[LABEL:.*]] -; CHECK-NEXT: jp [[LABEL]] -; CHECK: %bb2 -; CHECK: %bb1 -; CHECK: jmp -; -entry: - %0 = fpext float %x to double - %1 = fpext float %y to double - %2 = fmul double %0, %1 - %3 = fcmp une double %2, 0.000000e+00 - br i1 %3, label %bb1, label %bb2, !prof !1 - -bb1: - %4 = fadd double %2, -1.000000e+00 - br label %bb2 - -bb2: - %.0.in = phi double [ %4, %bb1 ], [ %2, %entry ] - %.0 = fptrunc double %.0.in to float - ret float %.0 -} - -!1 = !{!"branch_weights", i32 1, i32 1000} diff --git a/test/CodeGen/X86/x86-analyze-branch-jne-jp.ll b/test/CodeGen/X86/x86-analyze-branch-jne-jp.ll deleted file mode 100644 index fed985b7b56..00000000000 --- a/test/CodeGen/X86/x86-analyze-branch-jne-jp.ll +++ /dev/null @@ -1,21 +0,0 @@ -; RUN: llc -mcpu=corei7 -mtriple=x86_64-linux < %s | FileCheck %s -check-prefix=CHECK - -; Test if the negation of the non-equality check between floating points are -; translated to jnp followed by jne. - -; CHECK: jne -; CHECK-NEXT: jnp -define void @foo(float %f) { -entry: - %cmp = fcmp une float %f, 0.000000e+00 - br i1 %cmp, label %if.then, label %if.end - -if.then: - tail call void @a() - br label %if.end - -if.end: - ret void -} - -declare void @a()