JIT: import entire method for OSR, prune unneeded parts later by AndyAyersMS · Pull Request #83910 · dotnet/runtime

AndyAyersMS · 2023-03-24T22:58:32Z

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes#83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0 analysis. Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed. Fixesdotnet#83783. May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

ghost · 2023-03-24T23:07:28Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes #83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-03-24T23:12:00Z

FYI @jakobbotsch
cc @dotnet/jit-contrib

There are several ways to approach this -- in this version the original entry is unreachable, but we force it to stay around; alternatively, we could make the OSR entry unreachable and force it to stay around, and then swap entry points later.

This one is a bit less disruptive; if it pans out I can also reconcile it with the code where in OSR mode we forcibly import the original entry if we think the method might tail call (and which is why I know this approach will "work", since we already do this extra importation some of the time). For tail call cases we keep the original entry protected until after morph, so a bit later than what we do here. The protection mechanisms are different, so the tail call version still works.

This will be difficult to assess via SPMI as many OSR contexts will fail to replay. But some of them do and the diffs generally look encouraging.

It will also hurt OSR TP but again this may be hard to spot in SPMI. In actuality OSR compiles are pretty rare so I'm not that worried about the extra work.

A third option is to just compile the method normally and then once we're past morph say, rework the control flow to jump to the OSR entry from scratch. That would be more work because I'd have to revise the whole "OSR step block" scheme so that it could run later, in case the OSR entry happens to be in the middle of a bunch of try regions.

Much of what happens before morph is currently flow insensitive, so it doesn't matter that a big swath of blocks are unreachable. It might mess up early liveness, but if so we already have this problem in methods that might tail call.

This may fix some latent OSR perf issues, but I don't expect it to fix them all.

AndyAyersMS · 2023-03-25T02:08:16Z

Hmm, looks like a fairly persistent assert.

AndyAyersMS · 2023-03-25T06:31:01Z

Problem is that once you set BBF_DONT_REMOVE it is hard to safely un-set it. So I will just revise and use the existing artificial ref count solution. This will keep the extra IR and blocks around a bit longer but be simpler overall.

AndyAyersMS · 2023-03-25T06:56:00Z

/azp run runtime-jit-experimental

azure-pipelines · 2023-03-25T06:56:17Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2023-03-25T14:43:15Z

Test failures seem unrelated. Passed jit-experimental which has various forms of OSR stress.

jakobbotsch

LGTM! I thought this would be a much more involved change

jakobbotsch · 2023-03-25T17:05:29Z

 varDsc->lvIsOSRLocal = true;
-
- if (info.compPatchpointInfo->IsExposed(lclNum))
- {


Do we still need to communicate the exposure information in the patchpoint information?

No. However, I'll leave this as is for now so older jits can still do somewhat reasonable things with newer SPMI data.

AndyAyersMS · 2023-03-26T07:43:57Z

LGTM! I thought this would be a much more involved change

Luckily (I guess) we already had this capability for some OSR methods, so now we just use it for all of them.

When I changed the importation strategy for OSR in dotnet#83910 it exposed a latent issue -- small OSR locals must normalized on load. Fixesdotnet#83959.

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0. Fixes#83959. Fixes#83960.

AndyAyersMS · 2023-03-30T19:48:35Z

Also seeing a regression here (not (yet) autofiled): ubuntu x64

AndyAyersMS · 2023-03-24T22:58:32Z

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes#83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0 analysis. Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed. Fixesdotnet#83783. May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

ghost · 2023-03-24T23:07:28Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes #83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-03-24T23:12:00Z

FYI @jakobbotsch
cc @dotnet/jit-contrib

There are several ways to approach this -- in this version the original entry is unreachable, but we force it to stay around; alternatively, we could make the OSR entry unreachable and force it to stay around, and then swap entry points later.

This one is a bit less disruptive; if it pans out I can also reconcile it with the code where in OSR mode we forcibly import the original entry if we think the method might tail call (and which is why I know this approach will "work", since we already do this extra importation some of the time). For tail call cases we keep the original entry protected until after morph, so a bit later than what we do here. The protection mechanisms are different, so the tail call version still works.

This will be difficult to assess via SPMI as many OSR contexts will fail to replay. But some of them do and the diffs generally look encouraging.

It will also hurt OSR TP but again this may be hard to spot in SPMI. In actuality OSR compiles are pretty rare so I'm not that worried about the extra work.

A third option is to just compile the method normally and then once we're past morph say, rework the control flow to jump to the OSR entry from scratch. That would be more work because I'd have to revise the whole "OSR step block" scheme so that it could run later, in case the OSR entry happens to be in the middle of a bunch of try regions.

Much of what happens before morph is currently flow insensitive, so it doesn't matter that a big swath of blocks are unreachable. It might mess up early liveness, but if so we already have this problem in methods that might tail call.

This may fix some latent OSR perf issues, but I don't expect it to fix them all.

AndyAyersMS · 2023-03-25T02:08:16Z

Hmm, looks like a fairly persistent assert.

AndyAyersMS · 2023-03-25T06:31:01Z

Problem is that once you set BBF_DONT_REMOVE it is hard to safely un-set it. So I will just revise and use the existing artificial ref count solution. This will keep the extra IR and blocks around a bit longer but be simpler overall.

AndyAyersMS · 2023-03-25T06:56:00Z

/azp run runtime-jit-experimental

azure-pipelines · 2023-03-25T06:56:17Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2023-03-25T14:43:15Z

Test failures seem unrelated. Passed jit-experimental which has various forms of OSR stress.

jakobbotsch

LGTM! I thought this would be a much more involved change

jakobbotsch · 2023-03-25T17:05:29Z

 varDsc->lvIsOSRLocal = true;
-
- if (info.compPatchpointInfo->IsExposed(lclNum))
- {


Do we still need to communicate the exposure information in the patchpoint information?

No. However, I'll leave this as is for now so older jits can still do somewhat reasonable things with newer SPMI data.

AndyAyersMS · 2023-03-26T07:43:57Z

LGTM! I thought this would be a much more involved change

Luckily (I guess) we already had this capability for some OSR methods, so now we just use it for all of them.

When I changed the importation strategy for OSR in dotnet#83910 it exposed a latent issue -- small OSR locals must normalized on load. Fixesdotnet#83959.

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0. Fixes#83959. Fixes#83960.

AndyAyersMS · 2023-03-30T19:48:35Z

Also seeing a regression here (not (yet) autofiled): ubuntu x64

AndyAyersMS · 2023-03-24T22:58:32Z

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes#83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0 analysis. Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed. Fixesdotnet#83783. May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

ghost · 2023-03-24T23:07:28Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes #83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-03-24T23:12:00Z

FYI @jakobbotsch
cc @dotnet/jit-contrib

There are several ways to approach this -- in this version the original entry is unreachable, but we force it to stay around; alternatively, we could make the OSR entry unreachable and force it to stay around, and then swap entry points later.

This one is a bit less disruptive; if it pans out I can also reconcile it with the code where in OSR mode we forcibly import the original entry if we think the method might tail call (and which is why I know this approach will "work", since we already do this extra importation some of the time). For tail call cases we keep the original entry protected until after morph, so a bit later than what we do here. The protection mechanisms are different, so the tail call version still works.

This will be difficult to assess via SPMI as many OSR contexts will fail to replay. But some of them do and the diffs generally look encouraging.

It will also hurt OSR TP but again this may be hard to spot in SPMI. In actuality OSR compiles are pretty rare so I'm not that worried about the extra work.

A third option is to just compile the method normally and then once we're past morph say, rework the control flow to jump to the OSR entry from scratch. That would be more work because I'd have to revise the whole "OSR step block" scheme so that it could run later, in case the OSR entry happens to be in the middle of a bunch of try regions.

Much of what happens before morph is currently flow insensitive, so it doesn't matter that a big swath of blocks are unreachable. It might mess up early liveness, but if so we already have this problem in methods that might tail call.

This may fix some latent OSR perf issues, but I don't expect it to fix them all.

AndyAyersMS · 2023-03-25T02:08:16Z

Hmm, looks like a fairly persistent assert.

AndyAyersMS · 2023-03-25T06:31:01Z

Problem is that once you set BBF_DONT_REMOVE it is hard to safely un-set it. So I will just revise and use the existing artificial ref count solution. This will keep the extra IR and blocks around a bit longer but be simpler overall.

AndyAyersMS · 2023-03-25T06:56:00Z

/azp run runtime-jit-experimental

azure-pipelines · 2023-03-25T06:56:17Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2023-03-25T14:43:15Z

Test failures seem unrelated. Passed jit-experimental which has various forms of OSR stress.

jakobbotsch

LGTM! I thought this would be a much more involved change

jakobbotsch · 2023-03-25T17:05:29Z

 varDsc->lvIsOSRLocal = true;
-
- if (info.compPatchpointInfo->IsExposed(lclNum))
- {


Do we still need to communicate the exposure information in the patchpoint information?

No. However, I'll leave this as is for now so older jits can still do somewhat reasonable things with newer SPMI data.

AndyAyersMS · 2023-03-26T07:43:57Z

LGTM! I thought this would be a much more involved change

Luckily (I guess) we already had this capability for some OSR methods, so now we just use it for all of them.

When I changed the importation strategy for OSR in dotnet#83910 it exposed a latent issue -- small OSR locals must normalized on load. Fixesdotnet#83959.

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0. Fixes#83959. Fixes#83960.

AndyAyersMS · 2023-03-30T19:48:35Z

Also seeing a regression here (not (yet) autofiled): ubuntu x64

AndyAyersMS · 2023-03-24T22:58:32Z

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes#83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0 analysis. Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed. Fixesdotnet#83783. May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

ghost · 2023-03-24T23:07:28Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes #83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-03-24T23:12:00Z

FYI @jakobbotsch
cc @dotnet/jit-contrib

There are several ways to approach this -- in this version the original entry is unreachable, but we force it to stay around; alternatively, we could make the OSR entry unreachable and force it to stay around, and then swap entry points later.

This one is a bit less disruptive; if it pans out I can also reconcile it with the code where in OSR mode we forcibly import the original entry if we think the method might tail call (and which is why I know this approach will "work", since we already do this extra importation some of the time). For tail call cases we keep the original entry protected until after morph, so a bit later than what we do here. The protection mechanisms are different, so the tail call version still works.

This will be difficult to assess via SPMI as many OSR contexts will fail to replay. But some of them do and the diffs generally look encouraging.

It will also hurt OSR TP but again this may be hard to spot in SPMI. In actuality OSR compiles are pretty rare so I'm not that worried about the extra work.

A third option is to just compile the method normally and then once we're past morph say, rework the control flow to jump to the OSR entry from scratch. That would be more work because I'd have to revise the whole "OSR step block" scheme so that it could run later, in case the OSR entry happens to be in the middle of a bunch of try regions.

Much of what happens before morph is currently flow insensitive, so it doesn't matter that a big swath of blocks are unreachable. It might mess up early liveness, but if so we already have this problem in methods that might tail call.

This may fix some latent OSR perf issues, but I don't expect it to fix them all.

AndyAyersMS · 2023-03-25T02:08:16Z

Hmm, looks like a fairly persistent assert.

AndyAyersMS · 2023-03-25T06:31:01Z

Problem is that once you set BBF_DONT_REMOVE it is hard to safely un-set it. So I will just revise and use the existing artificial ref count solution. This will keep the extra IR and blocks around a bit longer but be simpler overall.

AndyAyersMS · 2023-03-25T06:56:00Z

/azp run runtime-jit-experimental

azure-pipelines · 2023-03-25T06:56:17Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2023-03-25T14:43:15Z

Test failures seem unrelated. Passed jit-experimental which has various forms of OSR stress.

jakobbotsch

LGTM! I thought this would be a much more involved change

jakobbotsch · 2023-03-25T17:05:29Z

 varDsc->lvIsOSRLocal = true;
-
- if (info.compPatchpointInfo->IsExposed(lclNum))
- {


Do we still need to communicate the exposure information in the patchpoint information?

No. However, I'll leave this as is for now so older jits can still do somewhat reasonable things with newer SPMI data.

AndyAyersMS · 2023-03-26T07:43:57Z

LGTM! I thought this would be a much more involved change

Luckily (I guess) we already had this capability for some OSR methods, so now we just use it for all of them.

When I changed the importation strategy for OSR in dotnet#83910 it exposed a latent issue -- small OSR locals must normalized on load. Fixesdotnet#83959.

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0. Fixes#83959. Fixes#83960.

AndyAyersMS · 2023-03-30T19:48:35Z

Also seeing a regression here (not (yet) autofiled): ubuntu x64

AndyAyersMS · 2023-03-24T22:58:32Z

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes#83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0 analysis. Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed. Fixesdotnet#83783. May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

ghost · 2023-03-24T23:07:28Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes #83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-03-24T23:12:00Z

FYI @jakobbotsch
cc @dotnet/jit-contrib

There are several ways to approach this -- in this version the original entry is unreachable, but we force it to stay around; alternatively, we could make the OSR entry unreachable and force it to stay around, and then swap entry points later.

This one is a bit less disruptive; if it pans out I can also reconcile it with the code where in OSR mode we forcibly import the original entry if we think the method might tail call (and which is why I know this approach will "work", since we already do this extra importation some of the time). For tail call cases we keep the original entry protected until after morph, so a bit later than what we do here. The protection mechanisms are different, so the tail call version still works.

This will be difficult to assess via SPMI as many OSR contexts will fail to replay. But some of them do and the diffs generally look encouraging.

It will also hurt OSR TP but again this may be hard to spot in SPMI. In actuality OSR compiles are pretty rare so I'm not that worried about the extra work.

A third option is to just compile the method normally and then once we're past morph say, rework the control flow to jump to the OSR entry from scratch. That would be more work because I'd have to revise the whole "OSR step block" scheme so that it could run later, in case the OSR entry happens to be in the middle of a bunch of try regions.

Much of what happens before morph is currently flow insensitive, so it doesn't matter that a big swath of blocks are unreachable. It might mess up early liveness, but if so we already have this problem in methods that might tail call.

This may fix some latent OSR perf issues, but I don't expect it to fix them all.

AndyAyersMS · 2023-03-25T02:08:16Z

Hmm, looks like a fairly persistent assert.

AndyAyersMS · 2023-03-25T06:31:01Z

Problem is that once you set BBF_DONT_REMOVE it is hard to safely un-set it. So I will just revise and use the existing artificial ref count solution. This will keep the extra IR and blocks around a bit longer but be simpler overall.

AndyAyersMS · 2023-03-25T06:56:00Z

/azp run runtime-jit-experimental

azure-pipelines · 2023-03-25T06:56:17Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2023-03-25T14:43:15Z

Test failures seem unrelated. Passed jit-experimental which has various forms of OSR stress.

jakobbotsch

LGTM! I thought this would be a much more involved change

jakobbotsch · 2023-03-25T17:05:29Z

 varDsc->lvIsOSRLocal = true;
-
- if (info.compPatchpointInfo->IsExposed(lclNum))
- {


Do we still need to communicate the exposure information in the patchpoint information?

No. However, I'll leave this as is for now so older jits can still do somewhat reasonable things with newer SPMI data.

AndyAyersMS · 2023-03-26T07:43:57Z

LGTM! I thought this would be a much more involved change

Luckily (I guess) we already had this capability for some OSR methods, so now we just use it for all of them.

When I changed the importation strategy for OSR in dotnet#83910 it exposed a latent issue -- small OSR locals must normalized on load. Fixesdotnet#83959.

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0. Fixes#83959. Fixes#83960.

AndyAyersMS · 2023-03-30T19:48:35Z

Also seeing a regression here (not (yet) autofiled): ubuntu x64

AndyAyersMS · 2023-03-24T22:58:32Z

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes#83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0 analysis. Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed. Fixesdotnet#83783. May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

ghost · 2023-03-24T23:07:28Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes #83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-03-24T23:12:00Z

FYI @jakobbotsch
cc @dotnet/jit-contrib

There are several ways to approach this -- in this version the original entry is unreachable, but we force it to stay around; alternatively, we could make the OSR entry unreachable and force it to stay around, and then swap entry points later.

This one is a bit less disruptive; if it pans out I can also reconcile it with the code where in OSR mode we forcibly import the original entry if we think the method might tail call (and which is why I know this approach will "work", since we already do this extra importation some of the time). For tail call cases we keep the original entry protected until after morph, so a bit later than what we do here. The protection mechanisms are different, so the tail call version still works.

This will be difficult to assess via SPMI as many OSR contexts will fail to replay. But some of them do and the diffs generally look encouraging.

It will also hurt OSR TP but again this may be hard to spot in SPMI. In actuality OSR compiles are pretty rare so I'm not that worried about the extra work.

A third option is to just compile the method normally and then once we're past morph say, rework the control flow to jump to the OSR entry from scratch. That would be more work because I'd have to revise the whole "OSR step block" scheme so that it could run later, in case the OSR entry happens to be in the middle of a bunch of try regions.

Much of what happens before morph is currently flow insensitive, so it doesn't matter that a big swath of blocks are unreachable. It might mess up early liveness, but if so we already have this problem in methods that might tail call.

This may fix some latent OSR perf issues, but I don't expect it to fix them all.

AndyAyersMS · 2023-03-25T02:08:16Z

Hmm, looks like a fairly persistent assert.

AndyAyersMS · 2023-03-25T06:31:01Z

Problem is that once you set BBF_DONT_REMOVE it is hard to safely un-set it. So I will just revise and use the existing artificial ref count solution. This will keep the extra IR and blocks around a bit longer but be simpler overall.

AndyAyersMS · 2023-03-25T06:56:00Z

/azp run runtime-jit-experimental

azure-pipelines · 2023-03-25T06:56:17Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2023-03-25T14:43:15Z

Test failures seem unrelated. Passed jit-experimental which has various forms of OSR stress.

jakobbotsch

LGTM! I thought this would be a much more involved change

jakobbotsch · 2023-03-25T17:05:29Z

 varDsc->lvIsOSRLocal = true;
-
- if (info.compPatchpointInfo->IsExposed(lclNum))
- {


Do we still need to communicate the exposure information in the patchpoint information?

No. However, I'll leave this as is for now so older jits can still do somewhat reasonable things with newer SPMI data.

AndyAyersMS · 2023-03-26T07:43:57Z

LGTM! I thought this would be a much more involved change

Luckily (I guess) we already had this capability for some OSR methods, so now we just use it for all of them.

When I changed the importation strategy for OSR in dotnet#83910 it exposed a latent issue -- small OSR locals must normalized on load. Fixesdotnet#83959.

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0. Fixes#83959. Fixes#83960.

AndyAyersMS · 2023-03-30T19:48:35Z

Also seeing a regression here (not (yet) autofiled): ubuntu x64

AndyAyersMS · 2023-03-24T22:58:32Z

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes#83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0 analysis. Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed. Fixesdotnet#83783. May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

ghost · 2023-03-24T23:07:28Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes #83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-03-24T23:12:00Z

FYI @jakobbotsch
cc @dotnet/jit-contrib

There are several ways to approach this -- in this version the original entry is unreachable, but we force it to stay around; alternatively, we could make the OSR entry unreachable and force it to stay around, and then swap entry points later.

This one is a bit less disruptive; if it pans out I can also reconcile it with the code where in OSR mode we forcibly import the original entry if we think the method might tail call (and which is why I know this approach will "work", since we already do this extra importation some of the time). For tail call cases we keep the original entry protected until after morph, so a bit later than what we do here. The protection mechanisms are different, so the tail call version still works.

This will be difficult to assess via SPMI as many OSR contexts will fail to replay. But some of them do and the diffs generally look encouraging.

It will also hurt OSR TP but again this may be hard to spot in SPMI. In actuality OSR compiles are pretty rare so I'm not that worried about the extra work.

A third option is to just compile the method normally and then once we're past morph say, rework the control flow to jump to the OSR entry from scratch. That would be more work because I'd have to revise the whole "OSR step block" scheme so that it could run later, in case the OSR entry happens to be in the middle of a bunch of try regions.

Much of what happens before morph is currently flow insensitive, so it doesn't matter that a big swath of blocks are unreachable. It might mess up early liveness, but if so we already have this problem in methods that might tail call.

This may fix some latent OSR perf issues, but I don't expect it to fix them all.

AndyAyersMS · 2023-03-25T02:08:16Z

Hmm, looks like a fairly persistent assert.

AndyAyersMS · 2023-03-25T06:31:01Z

Problem is that once you set BBF_DONT_REMOVE it is hard to safely un-set it. So I will just revise and use the existing artificial ref count solution. This will keep the extra IR and blocks around a bit longer but be simpler overall.

AndyAyersMS · 2023-03-25T06:56:00Z

/azp run runtime-jit-experimental

azure-pipelines · 2023-03-25T06:56:17Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2023-03-25T14:43:15Z

Test failures seem unrelated. Passed jit-experimental which has various forms of OSR stress.

jakobbotsch

LGTM! I thought this would be a much more involved change

jakobbotsch · 2023-03-25T17:05:29Z

 varDsc->lvIsOSRLocal = true;
-
- if (info.compPatchpointInfo->IsExposed(lclNum))
- {


Do we still need to communicate the exposure information in the patchpoint information?

No. However, I'll leave this as is for now so older jits can still do somewhat reasonable things with newer SPMI data.

AndyAyersMS · 2023-03-26T07:43:57Z

LGTM! I thought this would be a much more involved change

Luckily (I guess) we already had this capability for some OSR methods, so now we just use it for all of them.

When I changed the importation strategy for OSR in dotnet#83910 it exposed a latent issue -- small OSR locals must normalized on load. Fixesdotnet#83959.

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0. Fixes#83959. Fixes#83960.

AndyAyersMS · 2023-03-30T19:48:35Z

Also seeing a regression here (not (yet) autofiled): ubuntu x64

AndyAyersMS · 2023-03-24T22:58:32Z

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes#83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0 analysis. Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed. Fixesdotnet#83783. May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

ghost · 2023-03-24T23:07:28Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

For OSR compiles, always import from the original entry point in addtion to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0
analysis.

Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed.

Fixes #83783.

May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-03-24T23:12:00Z

FYI @jakobbotsch
cc @dotnet/jit-contrib

There are several ways to approach this -- in this version the original entry is unreachable, but we force it to stay around; alternatively, we could make the OSR entry unreachable and force it to stay around, and then swap entry points later.

This one is a bit less disruptive; if it pans out I can also reconcile it with the code where in OSR mode we forcibly import the original entry if we think the method might tail call (and which is why I know this approach will "work", since we already do this extra importation some of the time). For tail call cases we keep the original entry protected until after morph, so a bit later than what we do here. The protection mechanisms are different, so the tail call version still works.

This will be difficult to assess via SPMI as many OSR contexts will fail to replay. But some of them do and the diffs generally look encouraging.

It will also hurt OSR TP but again this may be hard to spot in SPMI. In actuality OSR compiles are pretty rare so I'm not that worried about the extra work.

A third option is to just compile the method normally and then once we're past morph say, rework the control flow to jump to the OSR entry from scratch. That would be more work because I'd have to revise the whole "OSR step block" scheme so that it could run later, in case the OSR entry happens to be in the middle of a bunch of try regions.

Much of what happens before morph is currently flow insensitive, so it doesn't matter that a big swath of blocks are unreachable. It might mess up early liveness, but if so we already have this problem in methods that might tail call.

This may fix some latent OSR perf issues, but I don't expect it to fix them all.

AndyAyersMS · 2023-03-25T02:08:16Z

Hmm, looks like a fairly persistent assert.

AndyAyersMS · 2023-03-25T06:31:01Z

Problem is that once you set BBF_DONT_REMOVE it is hard to safely un-set it. So I will just revise and use the existing artificial ref count solution. This will keep the extra IR and blocks around a bit longer but be simpler overall.

AndyAyersMS · 2023-03-25T06:56:00Z

/azp run runtime-jit-experimental

azure-pipelines · 2023-03-25T06:56:17Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2023-03-25T14:43:15Z

Test failures seem unrelated. Passed jit-experimental which has various forms of OSR stress.

jakobbotsch

LGTM! I thought this would be a much more involved change

jakobbotsch · 2023-03-25T17:05:29Z

 varDsc->lvIsOSRLocal = true;
-
- if (info.compPatchpointInfo->IsExposed(lclNum))
- {


Do we still need to communicate the exposure information in the patchpoint information?

No. However, I'll leave this as is for now so older jits can still do somewhat reasonable things with newer SPMI data.

AndyAyersMS · 2023-03-26T07:43:57Z

LGTM! I thought this would be a much more involved change

Luckily (I guess) we already had this capability for some OSR methods, so now we just use it for all of them.

When I changed the importation strategy for OSR in dotnet#83910 it exposed a latent issue -- small OSR locals must normalized on load. Fixesdotnet#83959.

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0. Fixes#83959. Fixes#83960.

AndyAyersMS · 2023-03-30T19:48:35Z

Also seeing a regression here (not (yet) autofiled): ubuntu x64

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 24, 2023

ghost assigned AndyAyersMS Mar 24, 2023

This was referenced Mar 25, 2023
DataContractSerializerTests.DCS_MyPersonSurrogate_Stress failing in CI #35066
Closed
[release/6.0] Doublelinklist GC failures on Mono #83245
Closed
WasmTestOnBrowser-System.* test failures in CI #83655
Closed

revise how we protect fgEntryBB
cd0a0b0

add tests
79a79e9

AndyAyersMS marked this pull request as ready for review March 25, 2023 14:57

AndyAyersMS requested a review from jakobbotsch March 25, 2023 14:57

jakobbotsch approved these changes Mar 25, 2023
View reviewed changes

jakobbotsch reviewed Mar 25, 2023
View reviewed changes

AndyAyersMS merged commit f1f9fde into dotnet:mainMar 26, 2023

AndyAyersMS mentioned this pull request Mar 27, 2023
[jitosr_stress_random] Failures in System.Text related tests #83959
Closed

AndyAyersMS mentioned this pull request Mar 27, 2023
JIT: small OSR locals must be normalize on load #84000
Merged

kunalspathak mentioned this pull request Mar 30, 2023
[Perf] Windows/arm64: 2 Improvements on 3/26/2023 1:21:26 PM dotnet/perf-autofiling-issues#14620
Closed

AndyAyersMS mentioned this pull request Mar 30, 2023
On Stack Replacement Next Steps #33658
Open
72 tasks

AndyAyersMS mentioned this pull request Mar 30, 2023
I accidentaly found performance reggresion using"mandelbrot algorithm" #80757
Closed

ghost locked as resolved and limited conversation to collaborators Apr 30, 2023