Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

JIT: Fix operand evaluation order for GT_INDEX_ADDR (#20047)#20127

Merged
AndyAyersMS merged 1 commit into
dotnet:release/2.2from
AndyAyersMS:PortFixFor20040ToRelease2.2
Sep 29, 2018
Merged

JIT: Fix operand evaluation order for GT_INDEX_ADDR (#20047)#20127
AndyAyersMS merged 1 commit into
dotnet:release/2.2from
AndyAyersMS:PortFixFor20040ToRelease2.2

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

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

Closes #20040.

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 AndyAyersMS added this to the 2.2 milestone Sep 25, 2018
@AndyAyersMS
Copy link
Copy Markdown
Member Author

Port of this to 2.2 was approved by shiproom...

cc @RussKeldorph

See #20083 for now-abandoned port to 2.1.

@RussKeldorph
Copy link
Copy Markdown

I think x86 failures are due to a toolset update on the build machine

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Any ETA on resolving the toolset issue?

Aside from x86 issues, there are package restore failures and a few random disconnects. Am going to try retesting ...

@dotnet-bot retest this please

@RussKeldorph
Copy link
Copy Markdown

@AndyAyersMS I think we may just need #20142

@RussKeldorph
Copy link
Copy Markdown

Scratch that. Make it #20144.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Retesting now that #21044 is in.

@dotnet-bot retest this please

@AndyAyersMS
Copy link
Copy Markdown
Member Author

arm64 still unhappy:

error C3859: virtual memory range for PCH exceeded; please recompile with a command line option of '-Zm72' or greater [D:\j\workspace\arm64_cross_c---30d2bc78\bin\obj\Windows_NT.arm64.Checked\src\vm\wks\cee_wks.vcxproj

@AndyAyersMS
Copy link
Copy Markdown
Member Author

@RussKeldorph any concerns with me merging this? I don't think those last bits of CI are reliable.

@AndyAyersMS AndyAyersMS merged commit ad096c8 into dotnet:release/2.2 Sep 29, 2018
@AndyAyersMS AndyAyersMS deleted the PortFixFor20040ToRelease2.2 branch September 29, 2018 01:57
</ItemGroup>
<PropertyGroup>
<DebugType>None</DebugType>
<Optimize>True</Optimize>
Copy link
Copy Markdown

@briansull briansull Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should; this repro be compiled as "Optimized=true" ?
I thought this it required MinOpts.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

It does require minopts, but there's no C# option set that will enable minopts for a method.

So we either let tiered jitting or forced minopts runs cover it.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Actually I stand corrected -- marking the method with [MethodImplOptions(NoOptimization)] should do the trick.

I will update the test.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants