Fix silent bad codegen in signed comparison
authorMichelle McDaniel <adiaaida@gmail.com>
Mon, 11 Jul 2016 16:32:56 +0000 (09:32 -0700)
committerMichelle McDaniel <adiaaida@gmail.com>
Wed, 20 Jul 2016 18:13:45 +0000 (11:13 -0700)
commit243e7c4f661d5ad0217c9e743c391a0df43bb6a0
tree4fa2851b2d65dbd2074f463461acda716aaf9fcf
parent03250d34da85f23df0a8f7ad1c8b3760e0b73b1f
Fix silent bad codegen in signed comparison

While working on #5956, I uncovered a silent bad codegen bug in signed
comparison. The way the code was written, we'd do an unsigned set after a
signed compare/jump from the high comparison. However, in this case, we
need to make sure that we are honoring the sign of the comparison done on
the high part of the operation, and the lo comparison would still want to
be an unsigned compare followed by an unsigned set. This change splits
comparisons into two cases: 1) A signed comparison where we have two jumps
for the high part (GT_GT, GT_LT, GT_GE, and GT_LE), which require a signed
set if the high comparisons are true, and an unsigned set if not, and 2)
An unsigned comparison, or a comparison where the sign does not matter for
the compare (GT_NE, GT_EQ), which just require an unsigned set for both
the high and lo operations (the code we already had in genCompareLong).

When we compare longs, we don't need to have both a jg/ja and a jne
instruction for the hi compare for the case where we enregister the result
of the compare, since the set will do the right thing. We only need to
check the lo halves if the hi halves were equal.

For long compares where the result isn't stored in a register, we need to use
signed jumps for the high comparison, and unsigned jumps for the low compare.
Currently, we generate a signed jump in the code for GT_JTRUE that is used by
both compares. This change modifies the logic for these compares/JTRUEs.
We separate the logic into genJTrueLong. In genJTrueLong, we use similar
logic as the legacy backend for the jumps, using a signed jump after the
high compare when we have signed operands, and unsigned jump when we have
a unsigned operands, and an unsigned jump for the low compare.

This change also modifies genJumpKindsForTreeLongHi, since it is now only
used for the uncontained case, where we need to jump to the bbJumpDest if
the compare is true, so we need to use the actual jump type, rather than
just checking for inequality (as inequality only tells us if we need to
check the low halves, not if the condition is true or not).

This change also adds test cases for the two silent bad codegen cases this
commit fixes.
src/jit/codegen.h
src/jit/codegenlinear.h
src/jit/codegenxarch.cpp
tests/src/JIT/Regression/JitBlue/GitHub_6238/GitHub_6238.cs [new file with mode: 0644]
tests/src/JIT/Regression/JitBlue/GitHub_6238/GitHub_6238.csproj [new file with mode: 0644]
tests/src/JIT/Regression/JitBlue/GitHub_6238/app.config [new file with mode: 0644]
tests/src/JIT/Regression/JitBlue/GitHub_6238/project.json [new file with mode: 0644]
tests/src/JIT/Regression/JitBlue/GitHub_6239/GitHub_6239.cs [new file with mode: 0644]
tests/src/JIT/Regression/JitBlue/GitHub_6239/GitHub_6239.csproj [new file with mode: 0644]
tests/src/JIT/Regression/JitBlue/GitHub_6239/app.config [new file with mode: 0644]
tests/src/JIT/Regression/JitBlue/GitHub_6239/project.json [new file with mode: 0644]