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

[Cong Hou]

On Tue, Jul 21, 2015 at 3:17 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Jul-21, at 11:27, Cong Hou <congh at google.com> wrote:
>>
>> congh created this revision.
>> congh added reviewers: dexonsmith, davidxl.
>> congh added a subscriber: llvm-commits.
>>
>> Currently, AnalyzeBranch() fails non-equality comparison between floating points on X86 (see https://llvm.org/bugs/show_bug.cgi?id=23875). This is because this function can modify the branch by reversing the conditional jump and removing unconditional jump if there is a proper fall-through. However, in the case of non-equality comparison between floating points, this can turn the branch "unanalyzable". Consider the following case:
>>
>> jne   .BB1
>> jp    .BB1
>> jmp   .BB2
>> .BB1:
>> ...
>> .BB2:
>> ...
>>
>> AnalyzeBranch() will reverse "jp  .BB1" to "jnp  .BB2" and then "jmp .BB2" will be removed:
>>
>> jne   .BB1
>> jnp   .BB2
>> .BB1:
>> ...
>> .BB2:
>> ...
>>
>> However, AnalyzeBranch() cannot analyze this branch anymore as there are two conditional jumps with different targets. This may disable some optimizations like block-placement: in this case the fall-through behavior is enforced even if the fall-through block is very cold, which is suboptimal.
>>
>> Actually this optimization is also done in block-placement pass, which means we can remove this optimization from AnalyzeBranch(). However, currently X86::COND_NE_OR_P and X86::COND_NP_OR_E are not reversible: there is no defined negation conditions for them.
>>
>> In order to reverse them, this patch defines two new CondCode X86::COND_NEG_NE_OR_P and X86::COND_NEG_NP_OR_E
>
> In terms of naming, `COND_NP_AND_E` and `COND_NE_AND_P` make more
> sense to me, but maybe there's precedent for your names?

Here "negation" is not for the whole condition:

 if (!(a or b)) goto B1; else goto B2;

but only for the second condition:

if (a) { goto B1; } else if (!b) { goto B2; }

So COND_NP_AND_E is imprecise and can be confusing. Getting a precise
name for the new condition is difficult, and I have added comments to
explain the behavior of the new condition.

>
>> . It also defines how to synthesize instructions for them. Here only the second conditional jump is reversed. This is valid as we only need them to do this "unconditional jump removal" optimization.
>>
>> The test cases haven't been updated accordingly. If this design is OK I will do it later.
>>
>> http://reviews.llvm.org/D11393
>>
>> Files:
>>  lib/Target/X86/X86InstrInfo.cpp
>>  lib/Target/X86/X86InstrInfo.h
>>
>> <D11393.30275.patch>
>
> This sounds reasonable to me, but I'm not the right person to review
> X86 backend patches.  You might try to CC a few people that commit
> there regularly.

Sure, I will do that. Thanks!