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

Jit: fix a few issues with single def local tracking #10633

Merged
merged 2 commits into from
Apr 1, 2017

Conversation

AndyAyersMS
Copy link
Member

Fixes for three issues that came up in desktop testing.

  1. Don't track class types for locals when in import only mode,
    since the jit may be looking at an uninstantiated generic method.
    In these cases the jit's use of TYP_REF to represent CORINFO_TYPE_VAR
    confuses the tracking code. In import only mode the jit is not going
    to use the information, so there is no need to track it at all.

    Import only mode is tied to verification and CoreCLR runs in full
    trust mode, so no test was added.

  2. Allow lvaUpdateClass to be called more than once but assert
    that the second and subsequent calls provide the exact same update
    as the first call.

    This can happen for single def ref class locals whose definition
    block is reimported. Reimportation is triggered by some tolerable
    non-ref type mismatches at block joins and so ref type information
    should be consistent for each importation attempt.

    Add a couple of IL test cases translated from destkop MC++ tests
    that will trigger this kind of reimportation.

  3. Bail out of devirtualization (for desktop only) if the base class
    has precise initialization semantics, since desktop seems to trigger
    class initialization for ref type callvirts (contrary to ECMA355 I.8.9.5).
    See #4853 for some discussion. Since this is desktop only behavior
    no test was added.

Fixes for three issues that came up in desktop testing.

1. Don't track class types for locals when in import only mode,
since the jit may be looking at an uninstantiated generic method.
In these cases the jit's use of TYP_REF to represent CORINFO_TYPE_VAR
confuses the tracking code. In import only mode the jit is not going
to use the information, so there is no need to track it at all.

    Import only mode is tied to verification and CoreCLR runs in full
trust mode, so no test was added.

2. Allow `lvaUpdateClass` to be called more than once but assert
that the second and subsequent calls provide the exact same update
as the first call.

    This can happen for single def ref class locals whose definition
block is reimported. Reimportation is triggered by some tolerable
non-ref type mismatches at block joins and so ref type information
should be consistent for each importation attempt.

    Add a couple of IL test cases translated from destkop MC++ tests
that will trigger this kind of reimportation.

3. Bail out of devirtualization (for desktop only) if the base class
has precise initialization semantics, since desktop seems to trigger
class initialization for ref type callvirts (contrary to ECMA355 I.8.9.5).
See #4853 for some discussion. Since this is desktop only behavior
no test was added.
@AndyAyersMS
Copy link
Member Author

@briansull PTAL
cc @dotnet/jit-contrib

No diffs in CoreCLR on jit-diff over frameworks.

@briansull
Copy link

Looks Good

@AndyAyersMS
Copy link
Member Author

Looks like the new tests fail on x86. Not too surprising since I ported them from x64 MC++.

Let me see if I can fix them to be less arch-dependent.

@briansull
Copy link

One additional comment on this

Reimportation is triggered by some tolerable non-ref type mismatches at block joins and so ref type information should be consistent for each importation attempt.

I think that re-importation is also triggered when a null is push on the IL stack on one path and then on the alternate path a non-null ref type is pushed.

@AndyAyersMS
Copy link
Member Author

The cases I see in the importer that trigger reimportation do not involve ref types:

  • int32 and native int --> native int
  • double and float --> double
  • & and native int --> &
  • & and int32 --> &

@AndyAyersMS AndyAyersMS merged commit a7ab04c into dotnet:master Apr 1, 2017
@AndyAyersMS AndyAyersMS deleted the FixReimportIssue branch April 1, 2017 07:56
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
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.

4 participants