Elide redundant test instruction for signed comparison predicates

[ashay]

Prior to this patch, when emitting code for x64, if an instruction
updated the flags register and if the following instruction was an
equals or not-equals comparison with zero, the runtime elided the test
instruction for materializing the comparison.

Although correct, this is limiting, since it excludes the predicates
SGT, SGE, SLT, and SLE. This patch extends the optimization to allow the
omission of the test instruction for all signed integer comparisons.
We skip unsigned integer comparisons since unsigned comparisons with
zero can be evaluated earlier in the compilation process.

Fix #117866

[ashay]

I'd like to add a unit test but I am not sure what the right place is for adding code-gen tests. So far, I have the following patch, but if there's a better location / test, please let me know. Thanks!

diff --git a/src/tests/JIT/opt/Compares/compares.cs b/src/tests/JIT/opt/Compares/compares.cs
index f08c65bf4dc..f2231ab7840 100644
--- a/src/tests/JIT/opt/Compares/compares.cs
+++ b/src/tests/JIT/opt/Compares/compares.cs
@@ -345,6 +345,23 @@ public static void Le_else_double_int_consume(double f1, double f2, int a1, int
         consume<double>(a1, a2);
     }
 
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    [Theory]
+    [InlineData(10)]
+    public static void Check_elide_test_insn_on_x64(int x)
+    {
+        // X64-NOT:  test {{.*}}, {{.*}}
+        while (true)
+        {
+            x += 1;
+            if (x > 0)
+            {
+                break;
+            }
+        }
+        consume<int>(x);
+    }
+
     /* If/Else conditions that return. */
 
     [MethodImpl(MethodImplOptions.NoInlining)]

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[jakobbotsch]

The code below requires only the zero flag to be written, but that's not sufficient for these other conditions. I expect that's the cause of the test failures.

[ashay]

Thanks for the tip! I've added the checks for other flag bits and separated them by the condition code so that flag checks are specific to the requested condition code.

[ashay]

@dotnet-policy-service agree

[ashay]

I see almost all x64 tests fail, so it doesn't look like this change is safe. Perhaps we should check whether the test instruction's consumer looks up just OF, SF, ZF, and PF or whether it references more flags than those four. I'm peering over the x64 ISA using a theorem prover to see if I can find a counter example, but since that will take a while, I'm closing this PR for now.

[xtqqczze]

Perhaps we should check whether the test instruction's consumer looks up just OF, SF, ZF, and PF or whether it references more flags than those four.

adc, sbb, and various jump instructions all reference CF, which test sets to zero.

[ashay]

adc, sbb, and various jump instructions all reference CF, which test sets to zero.

Indeed, you're right, but I was wondering in the context of the SGE, SLE, SGT, and SLT comparisons. It seems like CF shouldn't matter for those comparisons.

[xtqqczze]

test instruction resets OF

[xtqqczze]

Suggested change

	if (DoesWriteSignFlag(lastIns) && DoesWriteOverflowFlag(lastIns) && IsFlagsAlwaysModified(id))
	if (DoesResetOverflowFlag(lastIns) && DoesWriteSignFlag(lastIns) && IsFlagsAlwaysModified(id))

[xtqqczze]

Suggested change

	if (DoesWriteZeroFlag(lastIns) && DoesWriteSignFlag(lastIns) && DoesWriteOverflowFlag(lastIns) &&
	if (DoesResetOverflowFlag(lastIns) && DoesWriteZeroFlag(lastIns) && DoesWriteSignFlag(lastIns) &&

[ashay]

Thanks for the suggested fix! I unfortunately botched the branch by force-pushing after rebasing with main, so I couldn't re-open this PR. Here's the new PR instead: #118806.

I agree that we should check whether lastIns resets OF and CF before removing the test instruction to match the semantics, but sadly, it doesn't resolve the original problem reported in #117866, since the add instruction doesn't always reset OF and CF.

Perhaps I misunderstand, but I wonder if there is a way to check the consumer of the flags to determine if we can ignore the updates to OF and CF. I can see how this would make the analysis fairly complicated since use-def chains for flags aren't explicitly encoded in the data flow, but if there's some existing logic that would help, please let me know. Thanks!