JIT/opt/Compares/compares should seemingly not be passing file-check on arm64

[jakobbotsch]

runtime/src/tests/JIT/opt/Compares/compares.cs

Lines 126 to 133 in 9e8d0a8

	[MethodImpl(MethodImplOptions.NoInlining)]
	public static void Ge_uint_consume(uint a1, uint a2)
	{
	//ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}}
	//ARM64-NEXT-FULL-LINE: csel {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, ge
	if (a1 >= a2) { a1 = 15; }
	consume<uint>(a1, a2);
	}

ge is a signed comparison. If I try this function locally via altjit I get:

; Assembly listing for method Program:Ge_uint_consume(uint,uint)
; Emitting BLENDED_CODE for generic ARM64 CPU - Windows
; optimized code
; fp based frame
; partially interruptible
; No PGO data
; invoked as altjit
; Final local variable assignments
;
;  V00 arg0         [V00,T00] (  6,  6   )     int  ->   x0
;  V01 arg1         [V01,T01] (  4,  4   )     int  ->   x1         single-def
;# V02 OutArgs      [V02    ] (  1,  1   )  lclBlk ( 0) [sp+00H]   "OutgoingArgSpace"
;
; Lcl frame size = 0

G_M25905_IG01:              ;; offset=0000H
        A9BF7BFD          stp     fp, lr, [sp, #-0x10]!
        910003FD          mov     fp, sp
                                                ;; size=8 bbWeight=1 PerfScore 1.50
G_M25905_IG02:              ;; offset=0008H
        528001E2          mov     w2, #15
        6B01001F          cmp     w0, w1
        1A802040          csel    w0, w2, w0, hs
        D2929602          movz    x2, #0x94B0      // code for Program:consume[uint](uint,uint)
        F2B96B42          movk    x2, #0xCB5A LSL #16
        F2CFFFC2          movk    x2, #0x7FFE LSL #32
        F9400042          ldr     x2, [x2]
        D63F0040          blr     x2
                                                ;; size=32 bbWeight=1 PerfScore 7.00
G_M25905_IG03:              ;; offset=0028H
        A8C17BFD          ldp     fp, lr, [sp], #0x10
        D65F03C0          ret     lr
                                                ;; size=8 bbWeight=1 PerfScore 2.00

; Total bytes of code 48, prolog size 8, PerfScore 15.30, instruction count 12, allocated bytes for code 48 (MethodHash=96c19ace) for method Program:Ge_uint_consume(uint,uint)
; ============================================================

So the JIT is emitting hs, an unsigned comparison, as expected. But then why is the test passing?

cc @markples @TIHan

[ghost]

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

Issue Details

---

runtime/src/tests/JIT/opt/Compares/compares.cs

Lines 126 to 133 in 9e8d0a8

	[MethodImpl(MethodImplOptions.NoInlining)]
	public static void Ge_uint_consume(uint a1, uint a2)
	{
	//ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}}
	//ARM64-NEXT-FULL-LINE: csel {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, ge
	if (a1 >= a2) { a1 = 15; }
	consume<uint>(a1, a2);
	}

ge is a signed comparison. If I try this function locally via altjit I get:

; Assembly listing for method Program:Ge_uint_consume(uint,uint)
; Emitting BLENDED_CODE for generic ARM64 CPU - Windows
; optimized code
; fp based frame
; partially interruptible
; No PGO data
; invoked as altjit
; Final local variable assignments
;
;  V00 arg0         [V00,T00] (  6,  6   )     int  ->   x0
;  V01 arg1         [V01,T01] (  4,  4   )     int  ->   x1         single-def
;# V02 OutArgs      [V02    ] (  1,  1   )  lclBlk ( 0) [sp+00H]   "OutgoingArgSpace"
;
; Lcl frame size = 0

G_M25905_IG01:              ;; offset=0000H
        A9BF7BFD          stp     fp, lr, [sp, #-0x10]!
        910003FD          mov     fp, sp
                                                ;; size=8 bbWeight=1 PerfScore 1.50
G_M25905_IG02:              ;; offset=0008H
        528001E2          mov     w2, #15
        6B01001F          cmp     w0, w1
        1A802040          csel    w0, w2, w0, hs
        D2929602          movz    x2, #0x94B0      // code for Program:consume[uint](uint,uint)
        F2B96B42          movk    x2, #0xCB5A LSL #16
        F2CFFFC2          movk    x2, #0x7FFE LSL #32
        F9400042          ldr     x2, [x2]
        D63F0040          blr     x2
                                                ;; size=32 bbWeight=1 PerfScore 7.00
G_M25905_IG03:              ;; offset=0028H
        A8C17BFD          ldp     fp, lr, [sp], #0x10
        D65F03C0          ret     lr
                                                ;; size=8 bbWeight=1 PerfScore 2.00

; Total bytes of code 48, prolog size 8, PerfScore 15.30, instruction count 12, allocated bytes for code 48 (MethodHash=96c19ace) for method Program:Ge_uint_consume(uint,uint)
; ============================================================

So the JIT is emitting hs, an unsigned comparison, as expected. But then why is the test passing?

cc @markples @TIHan

Author:	jakobbotsch
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[jakobbotsch]

One thing I notice when I run the test wrapper scripts locally is that they do not delete the produced JIT disasm file (i.e. the file that DOTNET_JitStdOutFile is pointing at). The JIT appends to this file so if you run the same test multiple times you end up with repeated sets of jitdisasm in this file, and FileCheck can then get a much wider range of ASM to match.

I'm not sure if this explains why the test passes in CI though, since I would expect the test only to run once there.

(Edit: opened #81538 to delete it beforehand)

[jakobbotsch]

Looks like //ARM64-NEXT-FULL-LINE should be //ARM64-FULL-LINE-NEXT, so seems like a test bug. cc @a74nh

[jakobbotsch]

Actually I'm not sure how that makes sense given that we have been hitting file check issues on those lines in both #81267 and #79283, but other places seem to be using ARM64-FULL-LINE-NEXT, so I'm at a loss.

[a74nh]

I find the whole file-check process quite fiddly to get working.

When run, all the assembly for every function in the test is dumped to a single file. Is it possible file check is matching the assembly for a previous function?
Regardless if that is the case or not, it would be reassuring if each function went to a different file.

[jakobbotsch]

we have been hitting file check issues on those lines in both #81267 and #79283

The failure we hit is in the first test case, and that one actually gets the directive right:

runtime/src/tests/JIT/opt/Compares/compares.cs

Line 85 in 9e8d0a8

//ARM64-FULL-LINE-NEXT: csel {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, eq

So looks like we just need to update the remaining test cases to use ARM64-FULL-LINE-NEXT.

[TIHan]

@a74nh - I think this is fair, we could split them up into separate files; wouldn't be too hard to do.

[a74nh]

ARM64-FULL-LINE-NEXT vs ARM64-NEXT-FULL-LINE

Is the first valid and the second invalid and ignored?

I wonder if it's worth expanding file-check so that any line with "//ARM64-" that doesn't match any valid define will cause an error in the testing (and same for X86 etc).

[markples]

I believe that we add anchors for the beginnings and ends of methods. Assuming that these can't be erroneously hit (seems reasonable?), I think that guarantees that we won't match code before a function to that function's checks. We could match code after a function to its checks, but then the end-of-method check would fail. That would be a poor experience, but it shouldn't incorrectly pass the full set of tests. Separating the files, assuming that dealing with multiple files works out, would eliminate that.

The ignored line behavior appears to be present in FileCheck itself, not that we added. My guess is that the reason for this behavior is that these check files are often invoked multiple times with different --check-prefix=<string> settings (like ARM64 vs X64), so such an error would flag the "other" checks. Unfortunately, FileCheck allows the initial prefix to contain -, so technically this example could be waiting for another invocation with --check-prefix=ARM64-NEXT-FULL-LINE. We could adopt "no dashes in top-level prefixes" as a rule and add a check for ourselves, but there might be resistance to contributing it. (We might want to visit the dash in FULL-LINE as part of this.)

A different strategy would be to implement our full-line extension as a "modifier" in FileCheck terminology: ARM64-NEXT{FULL-LINE}. Then for this particular example it would be clear that it goes last.

[JulieLeeMSFT]

Assigned to @a74nh. @TIHan will help to fix it.