JIT: Add missing `lvTracked` check in `optLocalHasNonLoopUses` by jakobbotsch · Pull Request #109505 · dotnet/runtime

jakobbotsch · 2024-11-04T13:24:14Z

optLocalHasNonLoopUses checks if a local might be used after a loop by making use of its liveness information, but it does not check lvTracked before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE.

Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

`optLocalHasNonLoopUses` checks if a local might be used after a loop by making use of its liveness information, but it does not check `lvTracked` before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE. Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

dotnet-policy-service · 2024-11-04T13:24:52Z

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

jakobbotsch · 2024-11-04T13:25:20Z

I added this check in #106637 and identified it as the cause of regressions like #106637 (comment).

jakobbotsch · 2024-11-04T15:15:35Z

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs.

I plan to add the necessary liveness computations as part of #106637, or maybe as a follow-up.

jakobbotsch · 2024-11-05T10:12:36Z

/ba-g MacOS 12 brownout and infra timeouts

jakobbotsch · 2024-11-04T13:24:14Z

optLocalHasNonLoopUses checks if a local might be used after a loop by making use of its liveness information, but it does not check lvTracked before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE.

Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

`optLocalHasNonLoopUses` checks if a local might be used after a loop by making use of its liveness information, but it does not check `lvTracked` before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE. Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

dotnet-policy-service · 2024-11-04T13:24:52Z

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

jakobbotsch · 2024-11-04T13:25:20Z

I added this check in #106637 and identified it as the cause of regressions like #106637 (comment).

jakobbotsch · 2024-11-04T15:15:35Z

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs.

I plan to add the necessary liveness computations as part of #106637, or maybe as a follow-up.

jakobbotsch · 2024-11-05T10:12:36Z

/ba-g MacOS 12 brownout and infra timeouts

jakobbotsch · 2024-11-04T13:24:14Z

optLocalHasNonLoopUses checks if a local might be used after a loop by making use of its liveness information, but it does not check lvTracked before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE.

Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

`optLocalHasNonLoopUses` checks if a local might be used after a loop by making use of its liveness information, but it does not check `lvTracked` before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE. Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

dotnet-policy-service · 2024-11-04T13:24:52Z

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

jakobbotsch · 2024-11-04T13:25:20Z

I added this check in #106637 and identified it as the cause of regressions like #106637 (comment).

jakobbotsch · 2024-11-04T15:15:35Z

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs.

I plan to add the necessary liveness computations as part of #106637, or maybe as a follow-up.

jakobbotsch · 2024-11-05T10:12:36Z

/ba-g MacOS 12 brownout and infra timeouts

jakobbotsch · 2024-11-04T13:24:14Z

optLocalHasNonLoopUses checks if a local might be used after a loop by making use of its liveness information, but it does not check lvTracked before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE.

Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

`optLocalHasNonLoopUses` checks if a local might be used after a loop by making use of its liveness information, but it does not check `lvTracked` before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE. Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

dotnet-policy-service · 2024-11-04T13:24:52Z

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

jakobbotsch · 2024-11-04T13:25:20Z

I added this check in #106637 and identified it as the cause of regressions like #106637 (comment).

jakobbotsch · 2024-11-04T15:15:35Z

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs.

I plan to add the necessary liveness computations as part of #106637, or maybe as a follow-up.

jakobbotsch · 2024-11-05T10:12:36Z

/ba-g MacOS 12 brownout and infra timeouts

jakobbotsch · 2024-11-04T13:24:14Z

optLocalHasNonLoopUses checks if a local might be used after a loop by making use of its liveness information, but it does not check lvTracked before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE.

Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

`optLocalHasNonLoopUses` checks if a local might be used after a loop by making use of its liveness information, but it does not check `lvTracked` before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE. Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

dotnet-policy-service · 2024-11-04T13:24:52Z

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

jakobbotsch · 2024-11-04T13:25:20Z

I added this check in #106637 and identified it as the cause of regressions like #106637 (comment).

jakobbotsch · 2024-11-04T15:15:35Z

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs.

I plan to add the necessary liveness computations as part of #106637, or maybe as a follow-up.

jakobbotsch · 2024-11-05T10:12:36Z

/ba-g MacOS 12 brownout and infra timeouts

jakobbotsch · 2024-11-04T13:24:14Z

optLocalHasNonLoopUses checks if a local might be used after a loop by making use of its liveness information, but it does not check lvTracked before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE.

Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

`optLocalHasNonLoopUses` checks if a local might be used after a loop by making use of its liveness information, but it does not check `lvTracked` before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE. Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

dotnet-policy-service · 2024-11-04T13:24:52Z

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

jakobbotsch · 2024-11-04T13:25:20Z

I added this check in #106637 and identified it as the cause of regressions like #106637 (comment).

jakobbotsch · 2024-11-04T15:15:35Z

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs.

I plan to add the necessary liveness computations as part of #106637, or maybe as a follow-up.

jakobbotsch · 2024-11-05T10:12:36Z

/ba-g MacOS 12 brownout and infra timeouts

jakobbotsch · 2024-11-04T13:24:14Z

optLocalHasNonLoopUses checks if a local might be used after a loop by making use of its liveness information, but it does not check lvTracked before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE.

Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

`optLocalHasNonLoopUses` checks if a local might be used after a loop by making use of its liveness information, but it does not check `lvTracked` before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE. Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

dotnet-policy-service · 2024-11-04T13:24:52Z

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

jakobbotsch · 2024-11-04T13:25:20Z

I added this check in #106637 and identified it as the cause of regressions like #106637 (comment).

jakobbotsch · 2024-11-04T15:15:35Z

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs.

I plan to add the necessary liveness computations as part of #106637, or maybe as a follow-up.

jakobbotsch · 2024-11-05T10:12:36Z

/ba-g MacOS 12 brownout and infra timeouts

jakobbotsch · 2024-11-04T13:24:14Z

optLocalHasNonLoopUses checks if a local might be used after a loop by making use of its liveness information, but it does not check lvTracked before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE.

Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

`optLocalHasNonLoopUses` checks if a local might be used after a loop by making use of its liveness information, but it does not check `lvTracked` before doing so. This is generally not a problem as there are other checks that the local is in SSA, and normally SSA implies lvTracked, but that's not the case for locals put into SSA by CSE. Since it is possible to have CSE defs in a loop with uses outside the loop, this check is necessary for correctness. It is expected to regress a bunch of IV opts cases, but in the future we can get those back by computing the necessary liveness when we insert into SSA.

dotnet-policy-service · 2024-11-04T13:24:52Z

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

jakobbotsch · 2024-11-04T13:25:20Z

I added this check in #106637 and identified it as the cause of regressions like #106637 (comment).

jakobbotsch · 2024-11-04T15:15:35Z

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs.

I plan to add the necessary liveness computations as part of #106637, or maybe as a follow-up.

jakobbotsch · 2024-11-05T10:12:36Z

/ba-g MacOS 12 brownout and infra timeouts

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 4, 2024

dotnet-policy-serviceBot assigned jakobbotsch Nov 4, 2024

jakobbotsch marked this pull request as ready for review November 4, 2024 15:14

jakobbotsch requested a review from AndyAyersMS November 4, 2024 15:15

AndyAyersMS approved these changes Nov 4, 2024
View reviewed changes

build-analysisBot mentioned this pull request Nov 4, 2024
The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008
Open
3 tasks

jakobbotsch merged commit 73e1976 into dotnet:mainNov 5, 2024

jakobbotsch deleted the lvTracked-fix branch November 5, 2024 10:12

jakobbotsch mentioned this pull request Nov 6, 2024
JIT: Put all CSEs into SSA #106637
Merged

github-actionsBot locked and limited conversation to collaborators Dec 6, 2024