See if this helps. Windows x64-centric, but steps are fairly similar on other arches or on Linux (and/or you can use an altjit).

cc @dotnet/jit-contrib -- we should have something like this in a more official place.

running jit diffs

Steps are

1. obtain jitutils and bootstrap
2. build base runtime and (optionally) build tests
3. build diff runtime
4. run jit-diffs

Presumes you have two clones of the runtime repo: base (on master) and diff (branched from same commit as master with only jit changes). You probably can get by with one runtime repo clone via git worktree; I haven't tried this.

Steps 1 and 2 don't need to be rerun when you are making incremental jit changes in the diff repo; just redo steps 3 and 4 to get new diffs.

If you are also altering framework or vm code in your diff clone, things are more complex; in step 4 you must run base and diff independently, each in its respective core_root, and then run jit-analyze manually on the two result directories.

1. obtain tools

git clone https://github.com/dotnet/jitutils
cd \repos\jitutils
bootstrap.cmd

2. build base repo runtime

cd \repos\base-runtime
build -c release

Then build the test environment only

cd src\coreclr
build-test.cmd x64 release skipmanaged skipnative

or optionally build the test environment and all the coreclr pri0 tests

build-test.cmd x64 release

or optionally build the test environment and all the coreclr pri0 and pri1 tests

build-test.cmd x64 release -priority 1

Then build a checked runtime and test environment

build.cmd x64 checked skiptests skipbuildpackages
build-test.cmd x64 checked skipmanaged skipnative

3. build diff repo runtime

First time:

cd \repos\diff-runtime
build -c Release
cd src\coreclr
build.cmd x64 checked skiptests skipbuildpackages
build-test.cmd x64 checked skipmanaged skipnative

Subsequently (when you've just changed jit files), you can skip the release build:

cd \repos\new-runtime\src\coreclr
build.cmd x64 checked skiptests skipbuildpackages
build-test.cmd x64 checked skipmanaged skipnative

4. run jit-dffs

This is what I commonly run (PMI diffs across all FX).

jit-diff diff --pmi --base --base_root \repos\base-runtime --diff --diff_root \repos\diff-runtime  --core_root \repos\base-runtime\artifacts\tests\coreclr\Windows_NT.x64.Release\Tests\Core_Root -f

We should fix jit-diff so it can find core_root on its own.

You can also use a checked runtime as the core_root, but it's considerably slower and you will see debug assert codegen in SPC.

For other options try

jit-diff diff --help

For instance, to look at diffs in a particular assembly (say test cases you've created):

jit-diff diff --pmi  --base --base_root \repos\base-runtime   --diff --diff_root \repos\diff-runtime   --core_root \repos\base-runtime\artifacts\tests\coreclr\Windows_NT.x64.Release\Tests\Core_Root   --assembly <path to your assembly>

If you built tests in step 2 above, you can also diff over the tests, or just the benchmark subset. Diffing over all tests is sometimes a bit noisy, there are "expected failures", but they should be common to base and diff.

I ended up following the steps mentioned by @EgorBo in:
https://gist.github.com/EgorBo/044453d74d2a759113d3bff2f5f7e348

What I get is:

$ ~/projects/public/jitutils/bin/jit-analyze -b /tmp/jitdiff/dasmset_1/base -d /tmp/jitdiff/dasmset_2/base -r -c 15
Found 5 files with textual diffs.

Summary of Code Size diffs:
(Lower is better)

Total bytes of diff: 0 (0.000% of base)

0 total files with Code Size differences (0 improved, 0 regressed), 161 unchanged.

0 total methods with Code Size differences (0 improved, 0 regressed), 291049 unchanged.

5 files had text diffs but no metric diffs.
System.Private.CoreLib.dasm had 2510 diffs
System.Private.Uri.dasm had 140 diffs
System.Diagnostics.Process.dasm had 70 diffs
System.Private.Xml.Linq.dasm had 58 diffs
System.Management.dasm had 4 diffs

Is that the sort of output you accept/expect?

I can also follow the instructions by @AndyAyersMS (I will do it anyway for practice).

Also, the failing check isn't making a lot of sense to me.
Is there someone who can tell me if/how this is directly related to the PR itself?

Yes, that's the kind of output we're looking for. It tells us there likely aren't any examples of this in the framework assemblies.

So you might consider adding a new test, or adding an example of this to some existing test.

Seems like NOT(NOT(x)) should get similar treatment?

Seems like NOT(NOT(x)) should get similar treatment?

Isn't it useful to normalize booleans (e.g. from pinvokes) to 0..1 ?

However, it seems Roslyn already optimizes NOT(NOT(x)) so the normalization trick doesn't work

byte pinvoke_bool = 2; // e.g. a bool from a pinvoke
bool tmp = *(bool*) &pinvoke_bool;
tmp = !!tmp; // normalize
Console.WriteLine(*((byte*)&tmp)); // stil prints 2

See another trick;

Yes, that's the kind of output we're looking for. It tells us there likely aren't any examples of this in the framework assemblies.

So you might consider adding a new test, or adding an example of this to some existing test.

Seems like NOT(NOT(x)) should get similar treatment?

As for adding a specific test, sure I'll give it a try.

I assume it's OK if the test is part of the PR and then doesn't appear in the JIT diff?

Kind of new to this, trying to wrap my head as to what is considered the standard procedure here.

You can run jit diffs over all the tests but since we usually use the "before" runtime repo as the baseline, this won't show diffs in your newly added tests. But you may discover cases in already existing tests.

I usually just run diffs on new tests in isolation, via the --assembly arg to jit-diff (see last bit of my note above).

seems Roslyn already optimizes NOT(NOT(x))

You can separate the NOTs by an inline boundary to keep Roslyn from messing with them.

@damageboy, are you still working on this?

@damageboy, are you still working on this?

Well, last I left this, there was little to do, but I forgot to push it in terms of discussion and pinging people.

I can drop this, or try to restart for 6.0

@AndyAyersMS, how should @damageboy proceed? Thanks.

I noticed that this pattern sometimes is used to blame jit for lack of peephole optimizations 😄 so would be nice to merge especially because the change is just a few lines of code

The change itself looks fine, so please do restart.

You will need to move the new test under src\tests.

I've rebased everything but still having some build issues with the tests per-se.

I'll give it another try on a different machine.

The failing test on AZP seem... unrelated? How should I proceed?

Also, since Roslyn optimizes double NOT I'd allow it in JIT too, something like this:

        case GT_NOT:
        case GT_NEG:
            // Remove double negation
            if (op1->OperIs(oper))
            {
                return op1->AsOp()->gtGetOp1();
            }

            /* Any constant cases should have been folded earlier */
            noway_assert(!op1->OperIsConst() || !opts.OptEnabled(CLFLG_CONSTANTFOLD) || optValnumCSE_phase);
            break;

Thanks for the comments, @EgorBo.

I think the current state of affairs addresses your feedback.

Hi, I think I'm done with this.
I had this working (including in the CI) with no errors, but then I added another test, and the next CI run seems to be mostly OK except for a single CI pipeline that timed out without failing any test.

Let me know what you want me to do with this...?

@AndyAyersMS: I've implemented the same logic for GT_NOT, but Roslyn is currently isn't generating MSIL that would trigger GT_NOT:

[MethodImpl(MethodImplOptions.AggressiveOptimization | MethodImplOptions.AggressiveInlining)]
static bool NegationBool(bool value) => !value;

  .method private hidebysig static bool  NegationBool(bool 'value') cil managed aggressiveinlining aggressiveoptimization
  {
    // Code size       5 (0x5)
    .maxstack  8
    IL_0000:  ldarg.0
    IL_0001:  ldc.i4.0
    IL_0002:  ceq
    IL_0004:  ret
  } // end of method doublenegate::NegationBool

How do you suggest I proceed?

an MSIL based test?
Is there a different secret handshake?

Here's a simple test case:

using System;
using System.Runtime.CompilerServices;

class X
{
     static int Not(int x) => ~x;

     [MethodImpl(MethodImplOptions.NoInlining)]
     static int NotNot(int x) => Not(Not(x));

     public static int Main()
     {
          return NotNot(100);
     }
}

we currently produce

; Assembly listing for method X:NotNot(int):int
; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
;  V00 arg0         [V00,T00] (  3,  3   )     int  ->  rcx
;# V01 OutArgs      [V01    ] (  1,  1   )  lclBlk ( 0) [rsp+0x00]   "OutgoingArgSpace"
;* V02 tmp1         [V02    ] (  0,  0   )     int  ->  zero-ref    "Inlining Arg"
;
; Lcl frame size = 0

G_M60739_IG01:
                                                ;; bbWeight=1    PerfScore 0.00
G_M60739_IG02:
       8BC1                 mov      eax, ecx
       F7D0                 not      eax
       F7D0                 not      eax
                                                ;; bbWeight=1    PerfScore 0.75
G_M60739_IG03:
       C3                   ret
                                                ;; bbWeight=1    PerfScore 1.00

@AndyAyersMS Thanks, I've tested this now locally, and I see the code triggered (e.g. during breakpoint) and working as expected, as well as JitDumps for the new test case in the doublenegate.cs file working as expected.

For the test code:

static bool Test3()
{
    var x = DoubleNotInt(6);
    var y = 
        DoubleNotInt(
            DoubleNotInt(
                DoubleNotInt(x)));
    var z = DoubleNotInt(_dummyValueInt);

   if (x == 6 && y == 6 && z == 6)
        return true;
    return false;
}       

[MethodImpl(MethodImplOptions.AggressiveOptimization | MethodImplOptions.AggressiveInlining)]
static int NotInt(int value) => ~value;

[MethodImpl(MethodImplOptions.AggressiveOptimization | MethodImplOptions.AggressiveInlining)]
static int DoubleNotInt(int value) => NotInt(NotInt(value));

I now see the following as part of the new JitDump:

*************** After end code gen, before unwindEmit()
G_M3856_IG01:        ; func=00, offs=000000H, size=0004H, bbWeight=1    PerfScore 1.25, gcVars= {}, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, gcvars, byref, nogc <-- Prolog IG

IN0007: 000000 push     rbp
IN0008: 000001 mov      rbp, rsp

G_M3856_IG02:        ; offs=000004H, size=0011H, bbWeight=1    PerfScore 3.50, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref, isz

IN0001: 000004 mov      rax, 0x7FA847B935E8
IN0002: 00000E mov      eax, dword ptr [rax]
IN0003: 000010 cmp      eax, 6
IN0004: 000013 jne      SHORT G_M3856_IG05

G_M3856_IG03:        ; offs=000015H, size=0005H, bbWeight=0.50 PerfScore 0.12, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref

IN0005: 000015 mov      eax, 1

G_M3856_IG04:        ; offs=00001AH, size=0002H, bbWeight=0.50 PerfScore 0.75, epilog, nogc, extend

IN0009: 00001A pop      rbp
IN000a: 00001B ret      

G_M3856_IG05:        ; offs=00001CH, size=0002H, bbWeight=0.50 PerfScore 0.12, gcVars= {}, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, gcvars, byref

IN0006: 00001C xor      eax, eax

G_M3856_IG06:        ; offs=00001EH, size=0002H, bbWeight=0.50 PerfScore 0.75, epilog, nogc, extend

IN000b: 00001E pop      rbp
IN000c: 00001F ret      

Is there anything else you would like to see that you feel is missing?

Thanks, this looks good.

We've just made a bunch of automation investments to simplify some aspects of running diffs, so you are welcome to try using those (you'd need to merge your local branch up to the tip and possibly rebuild the jit).

Running diffs should now be as simple as running

python .\src\coreclr\scripts\superpmi.py asmdiffs

from the root of the repo.

I don't expect any diffs from your changes above, but you never know.

Going to restart the failed test legs... not sure what is going on. Looks like lots of timeouts.

Wondering if these failures are actually related. We're doing this optimization even for debug codegen which is probably not a great idea. You should add opts.OptimizationEnabled() as one of the conditions, eg

if ((oper == GT_NEG) && op1->OperIs(GT_NEG) && opts.OptimizationEnabled())

also we know already oper == GT_NEG so you can drop that bit out.

if (op1->OperIs(GT_NEG) && opts.OptimizationEnabled())

and perhaps follow Egor's suggestion and fold the NOT/NEG cases together, via

if (op1->OperIs(oper) && opts.OptimizationEnabled())

Thanks, this looks good.

We've just made a bunch of automation investments to simplify some aspects of running diffs, so you are welcome to try using those (you'd need to merge your local branch up to the tip and possibly rebuild the jit).

Running diffs should now be as simple as running

python .\src\coreclr\scripts\superpmi.py asmdiffs

from the root of the repo.

I don't expect any diffs from your changes above, but you never know.

Will do and report here.

Wondering if these failures are actually related. We're doing this optimization even for debug codegen which is probably not a great idea. You should add opts.OptimizationEnabled() as one of the conditions, eg

if ((oper == GT_NEG) && op1->OperIs(GT_NEG) && opts.OptimizationEnabled())

also we know already oper == GT_NEG so you can drop that bit out.

if (op1->OperIs(GT_NEG) && opts.OptimizationEnabled())

and perhaps follow Egor's suggestion and fold the NOT/NEG cases together, via

if (op1->OperIs(oper) && opts.OptimizationEnabled())

Sounds good, will do.

Latest looks good.

I'd encourage you in the future to just let the PR accumulate commits. We require squash on merge so all this is just for PR purposes -- I find seeing the incremental progress helps me review things better.

Latest looks good.

I'd encourage you in the future to just let the PR accumulate commits. We require squash on merge so all this is just for PR purposes -- I find seeing the incremental progress helps me review things better.

Sure.
Still having issues getting the tests to run on Linux/Ubuntu 20.04, weird filesystem time modification test fails.

I'll also try to get the python superpmi.py thingy going, no such luck for now.

I'll also try to get the python superpmi.py thingy going, no such luck for now.

Since the changes look good I'm going to merge.

Keep us updated on how it's going with local testing and/or using the new spmi scripts; there may be some rough edges there we need to address.