[PATCH] D11393: [X86] Allow X86::COND_NE_OR_P and X86::COND_NP_OR_E to be reversed.

[Xinliang David Li via llvm-commits]

On Thu, Jan 14, 2016 at 1:17 PM, Cong Hou <congh at google.com> wrote:
> congh added inline comments.
>
> ================
> Comment at: lib/Target/X86/X86InstrInfo.cpp:4055
> @@ +4054,3 @@
> +      // is named COND_P_AND_NE.
> +      BranchCode = X86::COND_P_AND_NE;
> +    } else if ((OldBranchCode == X86::COND_NP && BranchCode == X86::COND_NE) ||
> ----------------
> davidxl wrote:
>> congh wrote:
>> > davidxl wrote:
>> > > congh wrote:
>> > > > davidxl wrote:
>> > > > > For B1, the condition is NP_OR_E, so why not using COND_NP_OR_E as the branch code? Is the new code needed? (after swapping TBB and FBB)
>> > > > This is actually why this patch is created: COND_NP_OR_E is equivalent to COND_P_AND_NE, but the latter has the second condition reversed based on the former. COND_NP_OR_E has a JNP and JE, while COND_P_AND_NE has a JNP and JNE, or JE and JP. COND_NP_OR_E always has two identical targets, but COND_P_AND_NE doesn't. So they are different patterns. We use different names so that in X86::GetOppositeBranchCondition() we can get the reverse condition code for each other.

>> > > I am not sure if we need to introduce the Opposite branch code for E_OR_NP.
>> > >
>> > > Example:
>> > >
>> > >   JE B1
>> > >   JP B2
>> > > B1 (fall through):
>> > >
>> > > B2:
>> > >
>> > > For this case, the Analyze branch can return success with the following tuple:
>> > > {BranchCode =  E_OR_NP, TBB = B1, FBB = B2}
>> > >
>> > > Later when insertBranch is called: there are two scenarios:
>> > > 1) same as above where B1 is the fall through. The inserted code will be the same as above
>> > > 2) B2 is the layout successor.  In this second case, the generated code sequence should look like:
>> > >    JNP B1
>> > >    JE   B1
>> > > B2 (fall through):
>> > > ..
>> > > B1:
>> > >
>> > > or
>> > >   JE B1
>> > >   JNP B1
>> > > B2:
>> > >   ..
>> > > B1:
>> > >
>> > > Does it make sense?
>> > In some places GetOppositeBranchCondition() is used to get the opposite branch code in order to generate an opposite branch. If we don't have an opposite branch code for COND_E_OR_NP, then how to reverse it?
>> GetOppositteBranchCondition for some reason is never called with COND_E_OR_NP before, do you know why this path was not triggered ?
> There are two places to reverse branches:
>
> 1. AnalyzeBranch(), in which GetOppositteBranchCondition() is called when there is a unconditional jump in the end of a MBB. If this is the case of a je+jnp branch, then a je+jp branch will be generated only based on jnp (GetOppositteBranchCondition returns jnp for a jp), as at this moment the COND_E_OR_NP pattern hasn't been recognized. If there is no jmp in the end,  GetOppositteBranchCondition() won't be called. A je+jp pattern is handled similarly. When there is no jmp following je+jnp, a COND_E_OR_NP pattern will be recognized.

Since COND_E_OR_NP is not expected to appear in code stream, so this
path will never trigger the GetOppositeBranchCondition to be called
with that opcode.

>
> 2. Block placement: in this pass COND_E_OR_NP  won't be handled with a check.
>

Is this part of code that calls 'ReverseBranchCondition' ?

 // If PrevBB has a two-way branch, try to re-order the branches
  // such that we branch to the successor with higher probability first.
      if (TBB && !Cond.empty() && FBB &&
          MBPI->getEdgeProbability(PrevBB, FBB) >
              MBPI->getEdgeProbability(PrevBB, TBB) &&
          !TII->ReverseBranchCondition(Cond)) {
        DEBUG(dbgs() << "Reverse order of the two branches: "


Suppose before block layout we have the following code sequence:

JCC1 BB1
JCC2 BB2

BB1 (original fall through, cold):
...
BB2: hot


After Block placement:

BB2 is moved to be the prevBB's layout successor: then the branch
sequence needs to be updated to (not optimal)

JCC1 BB1
JCC2 BB2
JUMP BB1
BB2:
 ...
BB1: ..

Introducing negate opcode COND_NCC1_AND_CC2 for the 'composite' branch
code COND_CC1_OR_NCC2 can achieve the this, but I think for composite
branch code COND_CC1_OR_NCC2, we don't need to reverse the condition
to achieve the same thing -- just teach InsertBranch to check which BB
is the layout successor and generate the optimal one:

JCC1 BB1
JNCC2 BB1
BB2: ..
BB1: ..


This requires analyze branch to set both TBB and FBB (to indicate it
is an explicit two way branch  just like
   JCC1 BB1
   JMP  BB2
...
 BB1:
 BB2:


David





>
> http://reviews.llvm.org/D11393
>
>
>