Fix weight computation in jit #47470

omajid · 2021-01-26T16:27:24Z

This appears to be a typo. weight1 is from dsc1 and weight2 is from dsc2. If we are checking whether dsc1 is a register arg, we should be adjusting weight1, not weight2.

This appears to be a typo. weight1 is from dsc1 and weight2 is from dsc2. If we checking whether dsc1 is a register arg, we should be adjusting weight1, not weight2.

AndyAyersMS · 2021-02-01T22:24:44Z

Thanks for spotting this.

Looks like I introduced this in dotnet/coreclr#19077.

The fix looks good- any chance you can run diffs? The change above only had diffs in 2 methods... curious if this undoes that.

AndyAyersMS

LGTM.

omajid · 2021-02-01T22:30:06Z

any chance you can run diffs?

Sorry, I am not sure what this means.

AndyAyersMS · 2021-02-01T22:33:21Z

Sorry, I am not sure what this means.

It means to assess what impact this change has on the code the jit generates.

If you have built locally for say x64, it should be as simple as running

src\coreclr\scripts\superpmi.py asmdiffs

I'm happy to run diffs for you, if you prefer.

omajid · 2021-02-01T22:44:55Z

Thanks for clarifying this. I would love to give this a shot and learn something new. Let me try this out.

AndyAyersMS · 2021-02-02T00:16:34Z

I ran it locally because I was curious, but will let you post the results ... I am a bit surprised by what I found.

omajid · 2021-02-02T21:19:24Z

Sorry for being so thick. I am not having any luck trying to get this running

This is what I did:

./build.sh --test -rc Checked
./src/coreclr/scripts/superpmi.py asmdiffs   -log_level debug

That leads to errors like

Base JIT Path: /home/omajid/devel/dotnet/runtime/artifacts/spmi/basejit/e7f4dcd2a58a5630da554b6a7f8ff094e9555164.Linux.x64.Checked/libclrjit.so
Diff JIT Path: /home/omajid/devel/dotnet/runtime/artifacts/bin/coreclr/Linux.x64.Checked/libclrjit.so
Using MCH files:
  /home/omajid/devel/dotnet/runtime/artifacts/spmi/mch/960894e2-ec41-4088-82bb-bdcbac4ac2d3.Linux.x64/tests.pmi.Linux.x64.checked.mch
  /home/omajid/devel/dotnet/runtime/artifacts/spmi/mch/960894e2-ec41-4088-82bb-bdcbac4ac2d3.Linux.x64/libraries.crossgen.Linux.x64.checked.mch
  /home/omajid/devel/dotnet/runtime/artifacts/spmi/mch/960894e2-ec41-4088-82bb-bdcbac4ac2d3.Linux.x64/libraries.pmi.Linux.x64.checked.mch
Using superpmi from product build location: /home/omajid/devel/dotnet/runtime/artifacts/bin/coreclr/Linux.x64.Checked/superpmi

Temp Location: /tmp/tmpexjy5hs1

Running asm diffs of /home/omajid/devel/dotnet/runtime/artifacts/spmi/mch/960894e2-ec41-4088-82bb-bdcbac4ac2d3.Linux.x64/tests.pmi.Linux.x64.checked.mch
Invoking: /home/omajid/devel/dotnet/runtime/artifacts/bin/coreclr/Linux.x64.Checked/superpmi -a -v ew -f /tmp/tmpexjy5hs1/tests.pmi.Linux.x64.checked.mch_fail.mcl -diffMCList /tmp/tmpexjy5hs1/tests.pmi.Linux.x64.checked.mch_diff.mcl -r /tmp/tmpexjy5hs1/repro -p /home/omajid/devel/dotnet/runtime/artifacts/spmi/basejit/e7f4dcd2a58a5630da554b6a7f8ff094e9555164.Linux.x64.Checked/libclrjit.so /home/omajid/devel/dotnet/runtime/artifacts/bin/coreclr/Linux.x64.Checked/libclrjit.so /home/omajid/devel/dotnet/runtime/artifacts/spmi/mch/960894e2-ec41-4088-82bb-bdcbac4ac2d3.Linux.x64/tests.pmi.Linux.x64.checked.mch
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)
Unknown error code 255

An strace shows that it's trying to find that file in ..../coreclr/Linux.x64.Checked/libcoredistools.so but it's actually in ..../coreclr/Linux.x64.Checked/R2RDump/libcoredistools.so:

$ find -iname libcoredistools.so
./artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/libcoredistools.so
./artifacts/bin/coreclr/Linux.x64.Checked/R2RDump/libcoredistools.so
$ grep libcoredistools.so strace.log | cut -d' ' -f2- | sort -u
openat(AT_FDCWD, "/home/omajid/devel/dotnet/runtime/artifacts/bin/coreclr/Linux.x64.Checked/libcoredistools.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/home/omajid/devel/dotnet/runtime/artifacts/bin/coreclr/Linux.x64.Checked/libcoredistools.so", O_RDONLY|O_CLOEXEC <unfinished ...>
write(2, "LoadLibrary(libcoredistools.so) "..., 51) = 51
write(2, "LoadLibrary(libcoredistools.so) "..., 51 <unfinished ...>

AndyAyersMS · 2021-02-02T21:38:01Z

Thanks for trying it out. Looks like we may need to adjust something for Linux?

cc @BruceForstall @dotnet/jit-contrib

I will dig in later today if Bruce doesn't get there first.

BruceForstall · 2021-02-03T00:49:11Z

Odd, it should find it in the Core_Root directory, and it does appear to be there. I don't know why it would look there. superpmi.py tries to set the current directory to the Core_Root path before running commands.

Do you see an output line like "Using coredistools found at" ?

I'll try to repro on Linux sometime (I haven't used the py script on Linux lately)

omajid · 2021-02-03T13:57:57Z

Do you see an output line like "Using coredistools found at" ?

Yes, I see this:

Using coredistools found at /home/omajid/devel/dotnet/runtime/artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/libcoredistools.so

And that's the correct location.

omajid · 2021-02-03T14:01:32Z

This comment seems rather suspicious:

runtime/src/coreclr/ToolBox/superpmi/superpmi/neardiffer.cpp

Lines 72 to 73 in 54f0d4b

    
           // Unix will require the full path to coredistools. Assume that the 
        
           // location is next to the full path to the superpmi.so.

The superpmi shared objects are in a different location than the "Using coredistools found at" location:

$ find -iname '*superpmi*so'
./artifacts/obj/coreclr/Linux.x64.Checked/ToolBox/superpmi/superpmi-shim-collector/libsuperpmi-shim-collector.so
./artifacts/obj/coreclr/Linux.x64.Checked/ToolBox/superpmi/superpmi-shim-counter/libsuperpmi-shim-counter.so
./artifacts/obj/coreclr/Linux.x64.Checked/ToolBox/superpmi/superpmi-shim-simple/libsuperpmi-shim-simple.so
./artifacts/bin/coreclr/Linux.x64.Checked/libsuperpmi-shim-simple.so
./artifacts/bin/coreclr/Linux.x64.Checked/libsuperpmi-shim-counter.so
./artifacts/bin/coreclr/Linux.x64.Checked/libsuperpmi-shim-collector.so

AndyAyersMS · 2021-02-03T16:44:07Z

@BruceForstall am trying to repro but also though I'd look into the minimal build steps required to be able to run this. Looks like we expect the test overlay (core_root) and so at least a full runtime build plus a test overlay setup?

Do we copy the SPMI shims into core_root?

AndyAyersMS · 2021-02-03T17:56:53Z

@omajid things work for me on Ubuntu if I do the following build steps:

./build.sh -arch x64 -subset clr -c Checked
./build.sh -arch x64 -subset libs -c Release
src/tests/build.sh x64 Checked generatelayoutonly

for output I get

Warning: deleting existing log file /home/andy/repos/runtime/artifacts/spmi/superpmi.log
================ Logging to /home/andy/repos/runtime/artifacts/spmi/superpmi.log
Using JIT/EE Version from jiteeversionguid.h: 960894e2-ec41-4088-82bb-bdcbac4ac2d3
Baseline hash: bb650672df2da1c7666e20d3abf713d050a954ca
Using baseline /home/andy/repos/runtime/artifacts/spmi/basejit/bb650672df2da1c7666e20d3abf713d050a954ca.Linux.x64.Checked/libclrjit.so
Using coredistools found at /home/andy/repos/runtime/artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/libcoredistools.so
Found download cache directory "/home/andy/repos/runtime/artifacts/spmi/mch/960894e2-ec41-4088-82bb-bdcbac4ac2d3.Linux.x64" and --force_download not set; skipping download
SuperPMI ASM diffs
Base JIT Path: /home/andy/repos/runtime/artifacts/spmi/basejit/bb650672df2da1c7666e20d3abf713d050a954ca.Linux.x64.Checked/libclrjit.so
Diff JIT Path: /home/andy/repos/runtime/artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/libclrjit.so
Using MCH files:
  /home/andy/repos/runtime/artifacts/spmi/mch/960894e2-ec41-4088-82bb-bdcbac4ac2d3.Linux.x64/libraries.pmi.Linux.x64.checked.mch
  /home/andy/repos/runtime/artifacts/spmi/mch/960894e2-ec41-4088-82bb-bdcbac4ac2d3.Linux.x64/tests.pmi.Linux.x64.checked.mch
  /home/andy/repos/runtime/artifacts/spmi/mch/960894e2-ec41-4088-82bb-bdcbac4ac2d3.Linux.x64/libraries.crossgen.Linux.x64.checked.mch
Running asm diffs of /home/andy/repos/runtime/artifacts/spmi/mch/960894e2-ec41-4088-82bb-bdcbac4ac2d3.Linux.x64/libraries.pmi.Linux.x64.checked.mch
Clean SuperPMI replay
... etc ...

and the files are co-located:

-rwxrw-r-- 1 andy andy 8137376 Nov  7 03:09 /home/andy/repos/runtime/artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/libcoredistools.so
-rwxr-xr-x 1 andy andy 8185792 Feb  3 08:48 /home/andy/repos/runtime/artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/libsuperpmi-shim-collector.so
-rwxr-xr-x 1 andy andy 8040960 Feb  3 08:48 /home/andy/repos/runtime/artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/libsuperpmi-shim-counter.so
-rwxr-xr-x 1 andy andy 7982632 Feb  3 08:49 /home/andy/repos/runtime/artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/libsuperpmi-shim-simple.so

omajid · 2021-02-03T18:49:14Z

Thanks!

I made some progress following your steps:

$ ./build.sh -subset clr -c Checked
$ ./build.sh -subset libs -c Release
$ ./src/tests/build.sh x64 Checked generatelayoutonly

libcoredistools.so is in the right place now:

$ find -iname libcoredistools.so                                                                         
./artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/R2RDump/libcoredistools.so                                                                        
./artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/libcoredistools.so                                                                                
./artifacts/bin/coreclr/Linux.x64.Checked/R2RDump/libcoredistools.so

But asmdiff still fails:

Running asm diffs of /home/omajid/devel/dotnet/runtime/artifacts/spmi/mch/960894e2-ec41-4088-82bb-bdcbac4ac2d3.Linux.x64/tests.pmi.Linux.x64.checked.mch      
Invoking: /home/omajid/devel/dotnet/runtime/artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/superpmi -a -v ew -f /tmp/tmpp4n3qj3y/tests.pmi.Linux.x64.checked.mch_fail.mcl -diffMCList /tmp/tmpp4n3qj3y/tests.pmi.Linux.x64.checked.mch_diff.mcl -r /tmp/tmpp4n3qj3y/repro -p /home/omajid/devel/dotnet/runtime/artifacts/spmi/basejit/e7f4dcd2a58a5630da554b6a7f8ff094e9555164.Linux.x64.Checked/libclrjit.so /home/omajid/devel/dotnet/runtime/artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/libclrjit.so /home/omajid/devel/dotnet/runtime/artifacts/spmi/mch/960894e2-ec41-4088-82bb-bdcbac4ac2d3.Linux.x64/tests.pmi.Linux.x64.checked.mch                                                                                                                                                 
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)                                                                                                    
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)                                                                                                    
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)                                                                                                    
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)                                                                                                    
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)                                                                                                    
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)                                                                                                    
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)                                                                                                    
ERROR: LoadLibrary(libcoredistools.so) failed (0x0000007e)

It looks like a dependency is missing:

$ ldd ./artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/libcoredistools.so                     
        linux-vdso.so.1 (0x00007ffcca1ed000)                                                                                                                  
        libz.so.1 => /lib64/libz.so.1 (0x00007f7826b0b000)                                                                                                    
        librt.so.1 => /lib64/librt.so.1 (0x00007f7826b00000)                                                                                                  
        libdl.so.2 => /lib64/libdl.so.2 (0x00007f7826af9000)                                                                                                  
        libtinfo.so.5 => not found                                                                                                                            
        libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f7826ad7000)                                                                                        
        libm.so.6 => /lib64/libm.so.6 (0x00007f7826991000)                                                                                                    
        libstdc++.so.6 => /lib64/libstdc++.so.6 (0x00007f78267a7000)                                                                                          
        libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f782678c000)                                                                                            
        libc.so.6 => /lib64/libc.so.6 (0x00007f78265c1000)                                                                                                    
        /lib64/ld-linux-x86-64.so.2 (0x00007f7827507000)      
$ sudo dnf provides '*libtinfo.so.5'                                                                     
ncurses-compat-libs-6.2-3.20200222.fc33.i686 : Ncurses compatibility libraries                                                                                
Repo        : fedora                                                                                                                                          
Matched from:                                                                                                                                                 
Provide    : libtinfo.so.5                                                                                                                                    
                                                                                                                                                              
ncurses-compat-libs-6.2-3.20200222.fc33.x86_64 : Ncurses compatibility libraries                                                                              
Repo        : fedora                                                                                                                                          
Matched from:                                                                                                                                                 
Other       : *libtinfo.so.5     
$ rpm -qf /usr/lib64/libtinfo.so.6
ncurses-libs-6.2-3.20200222.fc33.x86_64

Fedora seems to be using a newer version of ncurses (libtinfo.so.6). I suppose this libcoredistools.so is a prebuilt since it is using a library not even installed on the system?

For now, this gets me running:

sudo dnf install ncurses-compat-libs

omajid · 2021-02-03T18:50:01Z

Was this the surprising part?

Asm diffs summary:
  No asm diffs

BruceForstall · 2021-02-03T20:52:22Z

Looks like we expect the test overlay (core_root) and so at least a full runtime build plus a test overlay setup?

It shouldn't be required to have a Core_Root, although superpmi.py will create one if it's not there, just to copy over a coredistools, and so that dir can be used as the "current directory" when spawning superpmi.

Do we copy the SPMI shims into core_root?

I think the "generate layout" step of the Core_Root building process does that, but that's only necessary for collection, not replay/asmdiffs.

It looks like the main problem identified is that the prebuilt version of coredistools doesn't work on (some versions of?) Fedora without additional dependencies being installed. Note that coredistools is built out of https://github.com/dotnet/jitutils an an irregular basis, and then you just pick up the published nuget packages.

cc @echesakovMSFT

echesakov · 2021-02-03T20:57:02Z

I wonder whether we can make the coredistools binary depending on smaller subset of libraries.
The ncurses dependency seems to be unnecessary for a disassembler and, perhaps, can be disable with some CMAKE configuration variable.

AndyAyersMS · 2021-02-03T22:01:37Z

No asm diffs

Yes, you'd think if we took pains to handle cases like this specially it should matter.

Thanks for checking diffs.

omajid · 2021-02-03T22:49:19Z

Thanks for checking diffs.

Thanks for helping me learn this! Much appreciated!

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 26, 2021

Fix weight computation in jit

28fcc06

This appears to be a typo. weight1 is from dsc1 and weight2 is from dsc2. If we checking whether dsc1 is a register arg, we should be adjusting weight1, not weight2.

omajid force-pushed the jit-lclvars-weight-typo branch from d2ee67d to 28fcc06 Compare January 26, 2021 16:28

JulieLeeMSFT requested a review from AndyAyersMS February 1, 2021 21:18

AndyAyersMS approved these changes Feb 1, 2021

View reviewed changes

AndyAyersMS mentioned this pull request Feb 3, 2021

Dynamic PGO #43618

Closed

54 tasks

AndyAyersMS merged commit 4de28f3 into dotnet:master Feb 3, 2021

echesakov mentioned this pull request Feb 4, 2021

See if libcoredistools.so can be built with fewer dependencies dotnet/jitutils#314

Closed

ghost locked as resolved and limited conversation to collaborators Mar 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix weight computation in jit #47470

Fix weight computation in jit #47470

omajid commented Jan 26, 2021 •

edited

Loading

AndyAyersMS commented Feb 1, 2021

AndyAyersMS left a comment

omajid commented Feb 1, 2021

AndyAyersMS commented Feb 1, 2021

omajid commented Feb 1, 2021

AndyAyersMS commented Feb 2, 2021

omajid commented Feb 2, 2021

AndyAyersMS commented Feb 2, 2021

BruceForstall commented Feb 3, 2021

omajid commented Feb 3, 2021

omajid commented Feb 3, 2021

AndyAyersMS commented Feb 3, 2021

AndyAyersMS commented Feb 3, 2021

omajid commented Feb 3, 2021

omajid commented Feb 3, 2021

BruceForstall commented Feb 3, 2021

echesakov commented Feb 3, 2021

AndyAyersMS commented Feb 3, 2021

omajid commented Feb 3, 2021

Fix weight computation in jit #47470

Fix weight computation in jit #47470

Conversation

omajid commented Jan 26, 2021 • edited Loading

AndyAyersMS commented Feb 1, 2021

AndyAyersMS left a comment

Choose a reason for hiding this comment

omajid commented Feb 1, 2021

AndyAyersMS commented Feb 1, 2021

omajid commented Feb 1, 2021

AndyAyersMS commented Feb 2, 2021

omajid commented Feb 2, 2021

AndyAyersMS commented Feb 2, 2021

BruceForstall commented Feb 3, 2021

omajid commented Feb 3, 2021

omajid commented Feb 3, 2021

AndyAyersMS commented Feb 3, 2021

AndyAyersMS commented Feb 3, 2021

omajid commented Feb 3, 2021

omajid commented Feb 3, 2021

BruceForstall commented Feb 3, 2021

echesakov commented Feb 3, 2021

AndyAyersMS commented Feb 3, 2021

omajid commented Feb 3, 2021

omajid commented Jan 26, 2021 •

edited

Loading