JIT: Fix operand evaluation order for GT_INDEX_ADDR

[AndyAyersMS]

We need to evaluate the array operand first, and it's op1. So evaluate
in that order, and don't allow reversal.

Closes #20040.

[AndyAyersMS]

@briansull PTAL
cc @dotnet/jit-contrib

@dotnet-bot test Ubuntu x64 Checked minopts
@dotnet-bot test Windows_NT x64 Checked minopts
@dotnet-bot test Windows_NT x64 Checked corefx_minopts

Diffs are fairly minimal:

Total bytes of diff: -1187 (0.00% of base)
    diff is an improvement.

Top file regressions by size (bytes):
         139 : Microsoft.CodeAnalysis.dasm (0.01% of base)
         111 : Microsoft.CodeAnalysis.CSharp.dasm (0.00% of base)
          85 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.00% of base)
          68 : System.Collections.dasm (0.01% of base)
          47 : System.Reflection.Metadata.dasm (0.01% of base)

Top file improvements by size (bytes):
        -882 : System.Private.Xml.dasm (-0.02% of base)
        -193 : System.Data.Common.dasm (-0.01% of base)
        -147 : System.Private.CoreLib.dasm (0.00% of base)
         -80 : System.Net.Mail.dasm (-0.03% of base)
         -61 : System.Net.Http.dasm (-0.01% of base)

55 total files with size differences (36 improved, 19 regressed), 74 unchanged.

Top method regressions by size (bytes):
          73 : System.Private.CoreLib.dasm - ThreadLocal`1:GrowTable(byref,int):this (5 methods)
          70 : System.Private.CoreLib.dasm - DefaultBinder:BindToField(int,ref,ref,ref):ref:this
          56 : System.Private.CoreLib.dasm - DefaultBinder:BindToMethod(int,ref,byref,ref,ref,ref,byref):ref:this
          43 : System.Reflection.Metadata.dasm - MetadataAggregator:ToImmutable(ref):struct (5 methods)
          42 : System.Private.Xml.dasm - XmlNodeReaderNavigator:InitDecAttr():this

Top method improvements by size (bytes):
        -198 : System.Private.CoreLib.dasm - Vector`1:.ctor(ref,int):this (4 methods)
        -109 : System.Private.Xml.dasm - XmlEventCache:EventsToWriter(ref):this
         -48 : System.Private.Xml.dasm - BigNumber:DblToRgbFast(double,ref,byref,byref):bool
         -41 : System.Linq.Parallel.dasm - SortHelper`2:MergeSortCooperatively():this (5 methods)
         -36 : System.Private.CoreLib.dasm - Dictionary`2:OnDeserialization(ref):this (5 methods)

1147 total methods with size differences (771 improved, 376 regressed), 191947 unchanged.

[AndyAyersMS]

Seems like some kind of networking glitch -- dotnet restore is failing. Am going to retry one leg to see if this is some transient issue.

@dotnet-bot retest Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness

[briansull]

Looks Fine

[AndyAyersMS]

@dotnet/dnceng seeing a number of issues with package restore:

error MSB3073: The command ""D:\j\workspace\x86_checked_w---cbcbeaa3\Tools\dotnetcli\dotnet.exe" restore --packages "D:\j\workspace\x86_checked_w---cbcbeaa3\packages" --source https://dotnetfeed.blob.core.windows.net/dotnet-core/index.json --source https://dotnet.myget.org/F/dotnet-core/api/v3/index.json --source https://api.nuget.org/v3/index.json D:\j\workspace\x86_checked_w---cbcbeaa3\src\ToolBox\SOS\NETCore\SOS.NETCore.csproj /p:TargetGroup= /p:ConfigurationGroup= /p:ArchGroup= /p:OSGroup= /p:TargetFramework= " exited with code 1. [D:\j\workspace\x86_checked_w---cbcbeaa3\src\ToolBox\SOS\NETCore\SOS.NETCore.csproj]

[CarolEidt]

@dotnet/dnceng - I am seeing similar package restore issues on my PR #19658.

[CarolEidt]

LGTM

[AndyAyersMS]

Retry failed too, so it looks like a somewhat persistent issue...

10:57:57 D:\j\w\perf_perflab_---6af0d14a\Tools\dotnetcli\sdk\2.1.301\NuGet.targets(114,5): error : Failed to retrieve information about 'System.Globalization' from remote source 'https://api.nuget.org/v3-flatcontainer/system.globalization/index.json'. [D:\j\w\perf_perflab_---6af0d14a\src\ToolBox\SOS\NETCore\SOS.NETCore.csproj] [D:\j\w\perf_perflab_---6af0d14a\src\ToolBox\SOS\NETCore\SOS.NETCore.csproj]
10:57:57 D:\j\w\perf_perflab_---6af0d14a\Tools\dotnetcli\sdk\2.1.301\NuGet.targets(114,5): error : Response status code does not indicate success: 504 (Gateway Timeout). [D:\j\w\perf_perflab_---6af0d14a\src\ToolBox\SOS\NETCore\SOS.NETCore.csproj] [D:\j\w\perf_perflab_---6af0d14a\src\ToolBox\SOS\NETCore\SOS.NETCore.csproj]
10:57:57 

[dagood]

Filed https://github.com/dotnet/core-eng/issues/4214 (critical FR issue) to track that problem, I hit it in a different repo too for a different package.

[AndyAyersMS]

@dagood any update on the restore issue?

[dagood]

Sorry, I should have been clearer about the issue--I filed it to track the issue and raise awareness to FR, but I'm not investigating it.

I think it would be worth retrying now. When I first saw the issue, I hit the 504 on my own machine when I visited https://api.nuget.org/v3-flatcontainer/system.globalization/index.json, but now it loads properly.

[AndyAyersMS]

Retrying a few ...

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT arm Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness

[AndyAyersMS]

Restore seems fixed, but still seeing various odd issues:

• Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness

14:28:10 D:\j\w\perf_perflab_---1c2b0529>py "D:\j\w\perf_perflab_---1c2b0529\Microsoft.BenchView.JSONFormat\tools\submission-metadata.py" --name "coreclr private JIT: Fix operand evaluation order for GT_INDEX_ADDR" --user-email "dotnet-bot@microsoft.com" 
14:28:10 'py' is not recognized as an internal or external command,
14:28:10 operable program or batch file.

• OSX10.12 x64 Checked Innerloop Build and Test

00:00:00.956 [WS-CLEANUP] Deleting project workspace...
00:00:01.052 java.io.IOException: Failed to mkdirs: /User/helix-runner/home/j/workspace/dotnet_coreclr/master/x64_checked_osx10.12_innerloop_flow_prtest
00:00:01.052 	at hudson.FilePath.mkdirs(FilePath.java:1170)

Will launch some of the others that failed initially with restore problems

@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test
@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT arm64 Cross Debug Innerloop Build
@dotnet-bot test Windows_NT arm Cross Debug Innerloop Build

[AndyAyersMS]

Seems like the perf test issue is known: #20058

[AndyAyersMS]

Still an annoying guessing game at times to figure out how to restart jobs.

@dotnet-bot test Windows_NT x64 Checked corefx_minopts
@dotnet-bot test Windows_NT x86 Checked Innerloop Build and Test

[AndyAyersMS]

@dotnet-bot test Windows_NT x86 Checked no_tiered_compilation_innerloop Build and Test

[Chrisboh]

Hi @AndyAyersMS the Mac issue is also know and we are working on rolling out the fix now. We are tracking that issue with https://github.com/dotnet/core-eng/issues/4215

[AndyAyersMS]

Arm cross failure seems like a random CI glitch:

00:59:01.459   Restoring packages for D:\j\workspace\arm_cross_deb---a3e3dfd4\tests\src\Common\Coreclr.TestWrapper\Coreclr.TestWrapper.csproj...
00:59:01.747 FATAL: command execution failed
00:59:01.747 java.nio.channels.ClosedChannelException
00:59:01.748 	at org.jenkinsci.remoting.protocol.NetworkLayer.onRecvClosed(NetworkLayer.java:154)

so will retry.

@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test

Still can't figure out how to re-trigger "Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0)" -- anyone know how?

[AndyAyersMS]

Ooops, meant to redo this one

@dotnet-bot retest Windows_NT arm Cross Debug Innerloop Build

[BruceForstall]

trying...

test Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0)

[BruceForstall]

fwiw, I'd prefer to get rid of all the "friendly names" at which point it might be harder to read, but triggering jobs would be cut-and-paste

[AndyAyersMS]

I guess that should have been obvious....

[AndyAyersMS]

Or just have a retry checkbox ... ?

[AndyAyersMS]

Hmm, this one not triggering either...

test Windows_NT arm Cross Debug Innerloop Build

[BruceForstall]

Not so obvious, actually -- there were two spaces after the dash, but GitHub only shows one. I just fixed that, so ignore that previous sentence: #20064

[AndyAyersMS]

More random failures in CI -- from the Ubuntu Arm tests:

00:18:31.355   StringTests.cpp
00:18:31.822      Creating library D:/j/workspace/x86_checked_w---f4983b4f/bin/int/Native/src/Interop/DllImportAttribute/Simple/Checked/DllFileProbe.lib and object D:/j/workspace/x86_checked_w---f4983b4f/bin/int/Native/src/Interop/DllImportAttribute/Simple/Checked/DllFileProbe.exp
00:18:32.324 FATAL: command execution failed
00:18:32.326 java.nio.channels.ClosedChannelException
00:18:32.328 	at org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer.onReadClosed(ChannelApplicationLayer.java:208)

I'm going to give up on the various failing legs and just make sure the remaining pending minopts ones pass.

[AndyAyersMS]

CI status not getting updated. for some reason There is (one? several?) minopts failures in the x64 CoreFX minopts test run. I will try to repro this locally.

System.Collections.Tests.Dictionary_IDictionary_NonGeneric_Tests.IEnumerable_NonGeneric_Enumerator_Reset_ModifiedBeforeEnumeration_ThrowsInvalidOperationException(count: 1) [FAIL]
00:32:41.057         Assert.All() Failure: 1 out of 3 items in the collection did not pass.
00:32:41.058       System.Collections.Tests.Dictionary_IDictionary_NonGeneric_Tests.IEnumerable_NonGeneric_Enumerator_Reset_ModifiedBeforeEnumeration_ThrowsInvalidOperationException(count: 75) [FAIL]
00:32:41.058         [2]: Item: System.Collections.Tests.IEnumerable_NonGeneric_Tests+ModifyEnumerable
00:32:41.058              Xunit.Sdk.ThrowsException: Assert.Throws() Failure
00:32:41.058              Expected: typeof(System.InvalidOperationException)
00:32:41.058              Actual:   (No exception was thrown)

But looking at history under the rolling job it looks likely these same tests fail w/o this change.

[AndyAyersMS]

Looks like the Windows_NT x64 Checked Build and Test (Jit - CoreFx TieredCompilation=1 JITMinOpts=1) still uses tests\run-corefx-tests.py and has a version skew issue. Am going to try and run locally with the new way...

From netci.groovy, only corefx_innerloop uses the new mechanism.

                        if (doCoreFxTesting) {
                            if (scenario == 'corefx_innerloop') {
                                // Create CORE_ROOT and testhost
                                buildCommands += "build-test.cmd ${lowerConfiguration} ${arch} buildtesthostonly"                                
                                buildCommands += "tests\\runtest.cmd ${runtestArguments} CoreFXTestsAll"

                                // Archive and process (only) the test results
                                Utilities.addArchival(newJob, "bin/Logs/**/testResults.xml")
                                Utilities.addXUnitDotNETResults(newJob, "bin/Logs/**/testResults.xml")
                            }
                            else {
                                def workspaceRelativeFxRoot = "_/fx"
                                def absoluteFxRoot = "%WORKSPACE%\\_\\fx"
                                def fxBranch = getFxBranch(branch)

                                buildCommands += "python -u %WORKSPACE%\\tests\\scripts\\run-corefx-tests.py -arch ${arch} -ci_arch ${architecture} -build_type ${configuration} -fx_root ${absoluteFxRoot} -fx_branch ${fxBranch} -env_script ${envScriptPath}"

[AndyAyersMS]

Running locally with the new approach, I don't see the failures... however it is hard to be 100% sure that minopts has tunneled its way in. So need to confirm that.

System.Collections.Tests  Total: 30303, Errors: 0, Failed: 0, Skipped: 0, Time: 145.374s

Edit: peeked at the dotnet process using procexp and saw minopts was set in the environment.

[AndyAyersMS]

@dotnet/jit-contrib I think all the leg failures are accounted for. Not sure things are going to look any better with retests, but happy to do so if you think it's the right call.

[CarolEidt]

I have no issues with merging this.

[AndyAyersMS]

Consensus on jit team is to go ahead and merge.