Ignore PhiArgs without matching actual preds

[EgorBo]

Fixes#124507

A couple of diffs

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT’s assertion propagation through SSA PHI nodes to be more conservative when PHI arguments don’t correspond to actual CFG predecessors, and adds a regression test for #124507.

Changes:

• Add a guard in optVisitReachingAssertions to abort PHI-based assertion inference when a PhiArg references a non-predecessor block.
• Add a new JIT regression test Runtime_124507.
• Wire the new test into Regression_ro_2.csproj.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/coreclr/jit/compiler.hpp	Abort PHI reaching-assertions walk when a PHI arg pred block is not an actual predecessor.
src/tests/JIT/Regression/Regression_ro_2.csproj	Includes the new Runtime_124507 regression test source file.
src/tests/JIT/Regression/JitBlue/Runtime_124507/Runtime_124507.cs	Adds a reduced repro as an xUnit test entry point for #124507.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[EgorBo]

PTAL @AndyAyersMS @dotnet/jit-contrib
I tried your suggestion with just checking two bitvectors in the end instead of the second pred walk, but Copilot suggested that it might be more expensive as we PHI don't have too many preds usually, but bitvectors might be long (fgMaxBlocks).

Also, I was not brave enough to just ignore PhiArgs without matching pred blocks (but if I do it, I get -200kb diff improvement).

[AndyAyersMS]

Can you describe what goes wrong without this fix?

Naively I'd expect that if we considered assertions from preds that no longer reached we would only be more conservative.

[EgorBo]

@AndyAyersMS it turned out the actual bug was in optGetEdgeAssertions that returned incorrect assertions for an edge that doesn't exist (because of RBO) for that Phi.

I propose we still keep the optVisitReachingAssertions change (it's no longer necessary to fix the bug) since the regression is pretty small just in case. We can alter the PhiVN if we want and remove dead preds somewhere (e.g. in GlobalAP)

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[EgorBo]

@AndyAyersMS anything else? Diffs are relatively small (mostly from test collections)

[AndyAyersMS]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

[EgorBo]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

@AndyAyersMS That is a very valid point. In GlobalAP when we use facts and fold comparisons into true/false, we call Morph for them that can convert BBJ_COND into BBJ_ALWAYS (e.g. via fgFoldConditional) and it currently doesn't do anything with what assertion vectors are valid. I can also take a look

[EgorBo]

/ba-g deadletter

[dhartglassMSFT]

/backport to release/10.0

[github-actions]

Started backporting to release/10.0 (link to workflow run)

[github-actions]

@dhartglassMSFT backporting to release/10.0 failed, the patch most likely resulted in conflicts. Please backport manually!

git am output

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patchApplying: ignore PhiArgs without real predsUsing index info to reconstruct a base tree...M	src/coreclr/jit/compiler.hppA	src/tests/JIT/Regression/Regression_ro_2.csprojFalling back to patching base and 3-way merge...Auto-merging src/coreclr/jit/compiler.hppCONFLICT (content): Merge conflict in src/coreclr/jit/compiler.hppCONFLICT (modify/delete): src/tests/JIT/Regression/Regression_ro_2.csproj deleted in HEAD and modified in ignore PhiArgs without real preds. Version ignore PhiArgs without real preds of src/tests/JIT/Regression/Regression_ro_2.csproj left in tree.error: Failed to merge in the changes.hint: Use 'git am --show-current-patch=diff' to see the failed patchhint: When you have resolved this problem, run "git am --continue".hint: If you prefer to skip this patch, run "git am --skip" instead.hint: To restore the original branch and stop patching, run "git am --abort".hint: Disable this message with "git config set advice.mergeConflict false"Patch failed at 0001 ignore PhiArgs without real predsError: The process '/usr/bin/git' failed with exit code 128

Link to workflow output

[EgorBo]

Fixes#124507

A couple of diffs

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT’s assertion propagation through SSA PHI nodes to be more conservative when PHI arguments don’t correspond to actual CFG predecessors, and adds a regression test for #124507.

Changes:

• Add a guard in optVisitReachingAssertions to abort PHI-based assertion inference when a PhiArg references a non-predecessor block.
• Add a new JIT regression test Runtime_124507.
• Wire the new test into Regression_ro_2.csproj.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/coreclr/jit/compiler.hpp	Abort PHI reaching-assertions walk when a PHI arg pred block is not an actual predecessor.
src/tests/JIT/Regression/Regression_ro_2.csproj	Includes the new Runtime_124507 regression test source file.
src/tests/JIT/Regression/JitBlue/Runtime_124507/Runtime_124507.cs	Adds a reduced repro as an xUnit test entry point for #124507.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[EgorBo]

PTAL @AndyAyersMS @dotnet/jit-contrib
I tried your suggestion with just checking two bitvectors in the end instead of the second pred walk, but Copilot suggested that it might be more expensive as we PHI don't have too many preds usually, but bitvectors might be long (fgMaxBlocks).

Also, I was not brave enough to just ignore PhiArgs without matching pred blocks (but if I do it, I get -200kb diff improvement).

[AndyAyersMS]

Can you describe what goes wrong without this fix?

Naively I'd expect that if we considered assertions from preds that no longer reached we would only be more conservative.

[EgorBo]

@AndyAyersMS it turned out the actual bug was in optGetEdgeAssertions that returned incorrect assertions for an edge that doesn't exist (because of RBO) for that Phi.

I propose we still keep the optVisitReachingAssertions change (it's no longer necessary to fix the bug) since the regression is pretty small just in case. We can alter the PhiVN if we want and remove dead preds somewhere (e.g. in GlobalAP)

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[EgorBo]

@AndyAyersMS anything else? Diffs are relatively small (mostly from test collections)

[AndyAyersMS]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

[EgorBo]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

@AndyAyersMS That is a very valid point. In GlobalAP when we use facts and fold comparisons into true/false, we call Morph for them that can convert BBJ_COND into BBJ_ALWAYS (e.g. via fgFoldConditional) and it currently doesn't do anything with what assertion vectors are valid. I can also take a look

[EgorBo]

/ba-g deadletter

[dhartglassMSFT]

/backport to release/10.0

[github-actions]

Started backporting to release/10.0 (link to workflow run)

[github-actions]

@dhartglassMSFT backporting to release/10.0 failed, the patch most likely resulted in conflicts. Please backport manually!

git am output

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patchApplying: ignore PhiArgs without real predsUsing index info to reconstruct a base tree...M	src/coreclr/jit/compiler.hppA	src/tests/JIT/Regression/Regression_ro_2.csprojFalling back to patching base and 3-way merge...Auto-merging src/coreclr/jit/compiler.hppCONFLICT (content): Merge conflict in src/coreclr/jit/compiler.hppCONFLICT (modify/delete): src/tests/JIT/Regression/Regression_ro_2.csproj deleted in HEAD and modified in ignore PhiArgs without real preds. Version ignore PhiArgs without real preds of src/tests/JIT/Regression/Regression_ro_2.csproj left in tree.error: Failed to merge in the changes.hint: Use 'git am --show-current-patch=diff' to see the failed patchhint: When you have resolved this problem, run "git am --continue".hint: If you prefer to skip this patch, run "git am --skip" instead.hint: To restore the original branch and stop patching, run "git am --abort".hint: Disable this message with "git config set advice.mergeConflict false"Patch failed at 0001 ignore PhiArgs without real predsError: The process '/usr/bin/git' failed with exit code 128

Link to workflow output

[EgorBo]

Fixes#124507

A couple of diffs

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT’s assertion propagation through SSA PHI nodes to be more conservative when PHI arguments don’t correspond to actual CFG predecessors, and adds a regression test for #124507.

Changes:

• Add a guard in optVisitReachingAssertions to abort PHI-based assertion inference when a PhiArg references a non-predecessor block.
• Add a new JIT regression test Runtime_124507.
• Wire the new test into Regression_ro_2.csproj.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/coreclr/jit/compiler.hpp	Abort PHI reaching-assertions walk when a PHI arg pred block is not an actual predecessor.
src/tests/JIT/Regression/Regression_ro_2.csproj	Includes the new Runtime_124507 regression test source file.
src/tests/JIT/Regression/JitBlue/Runtime_124507/Runtime_124507.cs	Adds a reduced repro as an xUnit test entry point for #124507.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[EgorBo]

PTAL @AndyAyersMS @dotnet/jit-contrib
I tried your suggestion with just checking two bitvectors in the end instead of the second pred walk, but Copilot suggested that it might be more expensive as we PHI don't have too many preds usually, but bitvectors might be long (fgMaxBlocks).

Also, I was not brave enough to just ignore PhiArgs without matching pred blocks (but if I do it, I get -200kb diff improvement).

[AndyAyersMS]

Can you describe what goes wrong without this fix?

Naively I'd expect that if we considered assertions from preds that no longer reached we would only be more conservative.

[EgorBo]

@AndyAyersMS it turned out the actual bug was in optGetEdgeAssertions that returned incorrect assertions for an edge that doesn't exist (because of RBO) for that Phi.

I propose we still keep the optVisitReachingAssertions change (it's no longer necessary to fix the bug) since the regression is pretty small just in case. We can alter the PhiVN if we want and remove dead preds somewhere (e.g. in GlobalAP)

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[EgorBo]

@AndyAyersMS anything else? Diffs are relatively small (mostly from test collections)

[AndyAyersMS]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

[EgorBo]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

@AndyAyersMS That is a very valid point. In GlobalAP when we use facts and fold comparisons into true/false, we call Morph for them that can convert BBJ_COND into BBJ_ALWAYS (e.g. via fgFoldConditional) and it currently doesn't do anything with what assertion vectors are valid. I can also take a look

[EgorBo]

/ba-g deadletter

[dhartglassMSFT]

/backport to release/10.0

[github-actions]

Started backporting to release/10.0 (link to workflow run)

[github-actions]

@dhartglassMSFT backporting to release/10.0 failed, the patch most likely resulted in conflicts. Please backport manually!

git am output

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patchApplying: ignore PhiArgs without real predsUsing index info to reconstruct a base tree...M	src/coreclr/jit/compiler.hppA	src/tests/JIT/Regression/Regression_ro_2.csprojFalling back to patching base and 3-way merge...Auto-merging src/coreclr/jit/compiler.hppCONFLICT (content): Merge conflict in src/coreclr/jit/compiler.hppCONFLICT (modify/delete): src/tests/JIT/Regression/Regression_ro_2.csproj deleted in HEAD and modified in ignore PhiArgs without real preds. Version ignore PhiArgs without real preds of src/tests/JIT/Regression/Regression_ro_2.csproj left in tree.error: Failed to merge in the changes.hint: Use 'git am --show-current-patch=diff' to see the failed patchhint: When you have resolved this problem, run "git am --continue".hint: If you prefer to skip this patch, run "git am --skip" instead.hint: To restore the original branch and stop patching, run "git am --abort".hint: Disable this message with "git config set advice.mergeConflict false"Patch failed at 0001 ignore PhiArgs without real predsError: The process '/usr/bin/git' failed with exit code 128

Link to workflow output

[EgorBo]

Fixes#124507

A couple of diffs

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT’s assertion propagation through SSA PHI nodes to be more conservative when PHI arguments don’t correspond to actual CFG predecessors, and adds a regression test for #124507.

Changes:

• Add a guard in optVisitReachingAssertions to abort PHI-based assertion inference when a PhiArg references a non-predecessor block.
• Add a new JIT regression test Runtime_124507.
• Wire the new test into Regression_ro_2.csproj.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/coreclr/jit/compiler.hpp	Abort PHI reaching-assertions walk when a PHI arg pred block is not an actual predecessor.
src/tests/JIT/Regression/Regression_ro_2.csproj	Includes the new Runtime_124507 regression test source file.
src/tests/JIT/Regression/JitBlue/Runtime_124507/Runtime_124507.cs	Adds a reduced repro as an xUnit test entry point for #124507.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[EgorBo]

PTAL @AndyAyersMS @dotnet/jit-contrib
I tried your suggestion with just checking two bitvectors in the end instead of the second pred walk, but Copilot suggested that it might be more expensive as we PHI don't have too many preds usually, but bitvectors might be long (fgMaxBlocks).

Also, I was not brave enough to just ignore PhiArgs without matching pred blocks (but if I do it, I get -200kb diff improvement).

[AndyAyersMS]

Can you describe what goes wrong without this fix?

Naively I'd expect that if we considered assertions from preds that no longer reached we would only be more conservative.

[EgorBo]

@AndyAyersMS it turned out the actual bug was in optGetEdgeAssertions that returned incorrect assertions for an edge that doesn't exist (because of RBO) for that Phi.

I propose we still keep the optVisitReachingAssertions change (it's no longer necessary to fix the bug) since the regression is pretty small just in case. We can alter the PhiVN if we want and remove dead preds somewhere (e.g. in GlobalAP)

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[EgorBo]

@AndyAyersMS anything else? Diffs are relatively small (mostly from test collections)

[AndyAyersMS]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

[EgorBo]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

@AndyAyersMS That is a very valid point. In GlobalAP when we use facts and fold comparisons into true/false, we call Morph for them that can convert BBJ_COND into BBJ_ALWAYS (e.g. via fgFoldConditional) and it currently doesn't do anything with what assertion vectors are valid. I can also take a look

[EgorBo]

/ba-g deadletter

[dhartglassMSFT]

/backport to release/10.0

[github-actions]

Started backporting to release/10.0 (link to workflow run)

[github-actions]

@dhartglassMSFT backporting to release/10.0 failed, the patch most likely resulted in conflicts. Please backport manually!

git am output

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patchApplying: ignore PhiArgs without real predsUsing index info to reconstruct a base tree...M	src/coreclr/jit/compiler.hppA	src/tests/JIT/Regression/Regression_ro_2.csprojFalling back to patching base and 3-way merge...Auto-merging src/coreclr/jit/compiler.hppCONFLICT (content): Merge conflict in src/coreclr/jit/compiler.hppCONFLICT (modify/delete): src/tests/JIT/Regression/Regression_ro_2.csproj deleted in HEAD and modified in ignore PhiArgs without real preds. Version ignore PhiArgs without real preds of src/tests/JIT/Regression/Regression_ro_2.csproj left in tree.error: Failed to merge in the changes.hint: Use 'git am --show-current-patch=diff' to see the failed patchhint: When you have resolved this problem, run "git am --continue".hint: If you prefer to skip this patch, run "git am --skip" instead.hint: To restore the original branch and stop patching, run "git am --abort".hint: Disable this message with "git config set advice.mergeConflict false"Patch failed at 0001 ignore PhiArgs without real predsError: The process '/usr/bin/git' failed with exit code 128

Link to workflow output

[EgorBo]

Fixes#124507

A couple of diffs

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT’s assertion propagation through SSA PHI nodes to be more conservative when PHI arguments don’t correspond to actual CFG predecessors, and adds a regression test for #124507.

Changes:

• Add a guard in optVisitReachingAssertions to abort PHI-based assertion inference when a PhiArg references a non-predecessor block.
• Add a new JIT regression test Runtime_124507.
• Wire the new test into Regression_ro_2.csproj.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/coreclr/jit/compiler.hpp	Abort PHI reaching-assertions walk when a PHI arg pred block is not an actual predecessor.
src/tests/JIT/Regression/Regression_ro_2.csproj	Includes the new Runtime_124507 regression test source file.
src/tests/JIT/Regression/JitBlue/Runtime_124507/Runtime_124507.cs	Adds a reduced repro as an xUnit test entry point for #124507.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[EgorBo]

PTAL @AndyAyersMS @dotnet/jit-contrib
I tried your suggestion with just checking two bitvectors in the end instead of the second pred walk, but Copilot suggested that it might be more expensive as we PHI don't have too many preds usually, but bitvectors might be long (fgMaxBlocks).

Also, I was not brave enough to just ignore PhiArgs without matching pred blocks (but if I do it, I get -200kb diff improvement).

[AndyAyersMS]

Can you describe what goes wrong without this fix?

Naively I'd expect that if we considered assertions from preds that no longer reached we would only be more conservative.

[EgorBo]

@AndyAyersMS it turned out the actual bug was in optGetEdgeAssertions that returned incorrect assertions for an edge that doesn't exist (because of RBO) for that Phi.

I propose we still keep the optVisitReachingAssertions change (it's no longer necessary to fix the bug) since the regression is pretty small just in case. We can alter the PhiVN if we want and remove dead preds somewhere (e.g. in GlobalAP)

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[EgorBo]

@AndyAyersMS anything else? Diffs are relatively small (mostly from test collections)

[AndyAyersMS]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

[EgorBo]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

@AndyAyersMS That is a very valid point. In GlobalAP when we use facts and fold comparisons into true/false, we call Morph for them that can convert BBJ_COND into BBJ_ALWAYS (e.g. via fgFoldConditional) and it currently doesn't do anything with what assertion vectors are valid. I can also take a look

[EgorBo]

/ba-g deadletter

[dhartglassMSFT]

/backport to release/10.0

[github-actions]

Started backporting to release/10.0 (link to workflow run)

[github-actions]

@dhartglassMSFT backporting to release/10.0 failed, the patch most likely resulted in conflicts. Please backport manually!

git am output

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patchApplying: ignore PhiArgs without real predsUsing index info to reconstruct a base tree...M	src/coreclr/jit/compiler.hppA	src/tests/JIT/Regression/Regression_ro_2.csprojFalling back to patching base and 3-way merge...Auto-merging src/coreclr/jit/compiler.hppCONFLICT (content): Merge conflict in src/coreclr/jit/compiler.hppCONFLICT (modify/delete): src/tests/JIT/Regression/Regression_ro_2.csproj deleted in HEAD and modified in ignore PhiArgs without real preds. Version ignore PhiArgs without real preds of src/tests/JIT/Regression/Regression_ro_2.csproj left in tree.error: Failed to merge in the changes.hint: Use 'git am --show-current-patch=diff' to see the failed patchhint: When you have resolved this problem, run "git am --continue".hint: If you prefer to skip this patch, run "git am --skip" instead.hint: To restore the original branch and stop patching, run "git am --abort".hint: Disable this message with "git config set advice.mergeConflict false"Patch failed at 0001 ignore PhiArgs without real predsError: The process '/usr/bin/git' failed with exit code 128

Link to workflow output

[EgorBo]

Fixes#124507

A couple of diffs

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT’s assertion propagation through SSA PHI nodes to be more conservative when PHI arguments don’t correspond to actual CFG predecessors, and adds a regression test for #124507.

Changes:

• Add a guard in optVisitReachingAssertions to abort PHI-based assertion inference when a PhiArg references a non-predecessor block.
• Add a new JIT regression test Runtime_124507.
• Wire the new test into Regression_ro_2.csproj.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/coreclr/jit/compiler.hpp	Abort PHI reaching-assertions walk when a PHI arg pred block is not an actual predecessor.
src/tests/JIT/Regression/Regression_ro_2.csproj	Includes the new Runtime_124507 regression test source file.
src/tests/JIT/Regression/JitBlue/Runtime_124507/Runtime_124507.cs	Adds a reduced repro as an xUnit test entry point for #124507.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[EgorBo]

PTAL @AndyAyersMS @dotnet/jit-contrib
I tried your suggestion with just checking two bitvectors in the end instead of the second pred walk, but Copilot suggested that it might be more expensive as we PHI don't have too many preds usually, but bitvectors might be long (fgMaxBlocks).

Also, I was not brave enough to just ignore PhiArgs without matching pred blocks (but if I do it, I get -200kb diff improvement).

[AndyAyersMS]

Can you describe what goes wrong without this fix?

Naively I'd expect that if we considered assertions from preds that no longer reached we would only be more conservative.

[EgorBo]

@AndyAyersMS it turned out the actual bug was in optGetEdgeAssertions that returned incorrect assertions for an edge that doesn't exist (because of RBO) for that Phi.

I propose we still keep the optVisitReachingAssertions change (it's no longer necessary to fix the bug) since the regression is pretty small just in case. We can alter the PhiVN if we want and remove dead preds somewhere (e.g. in GlobalAP)

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[EgorBo]

@AndyAyersMS anything else? Diffs are relatively small (mostly from test collections)

[AndyAyersMS]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

[EgorBo]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

@AndyAyersMS That is a very valid point. In GlobalAP when we use facts and fold comparisons into true/false, we call Morph for them that can convert BBJ_COND into BBJ_ALWAYS (e.g. via fgFoldConditional) and it currently doesn't do anything with what assertion vectors are valid. I can also take a look

[EgorBo]

/ba-g deadletter

[dhartglassMSFT]

/backport to release/10.0

[github-actions]

Started backporting to release/10.0 (link to workflow run)

[github-actions]

@dhartglassMSFT backporting to release/10.0 failed, the patch most likely resulted in conflicts. Please backport manually!

git am output

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patchApplying: ignore PhiArgs without real predsUsing index info to reconstruct a base tree...M	src/coreclr/jit/compiler.hppA	src/tests/JIT/Regression/Regression_ro_2.csprojFalling back to patching base and 3-way merge...Auto-merging src/coreclr/jit/compiler.hppCONFLICT (content): Merge conflict in src/coreclr/jit/compiler.hppCONFLICT (modify/delete): src/tests/JIT/Regression/Regression_ro_2.csproj deleted in HEAD and modified in ignore PhiArgs without real preds. Version ignore PhiArgs without real preds of src/tests/JIT/Regression/Regression_ro_2.csproj left in tree.error: Failed to merge in the changes.hint: Use 'git am --show-current-patch=diff' to see the failed patchhint: When you have resolved this problem, run "git am --continue".hint: If you prefer to skip this patch, run "git am --skip" instead.hint: To restore the original branch and stop patching, run "git am --abort".hint: Disable this message with "git config set advice.mergeConflict false"Patch failed at 0001 ignore PhiArgs without real predsError: The process '/usr/bin/git' failed with exit code 128

Link to workflow output

[EgorBo]

Fixes#124507

A couple of diffs

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT’s assertion propagation through SSA PHI nodes to be more conservative when PHI arguments don’t correspond to actual CFG predecessors, and adds a regression test for #124507.

Changes:

• Add a guard in optVisitReachingAssertions to abort PHI-based assertion inference when a PhiArg references a non-predecessor block.
• Add a new JIT regression test Runtime_124507.
• Wire the new test into Regression_ro_2.csproj.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/coreclr/jit/compiler.hpp	Abort PHI reaching-assertions walk when a PHI arg pred block is not an actual predecessor.
src/tests/JIT/Regression/Regression_ro_2.csproj	Includes the new Runtime_124507 regression test source file.
src/tests/JIT/Regression/JitBlue/Runtime_124507/Runtime_124507.cs	Adds a reduced repro as an xUnit test entry point for #124507.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[EgorBo]

PTAL @AndyAyersMS @dotnet/jit-contrib
I tried your suggestion with just checking two bitvectors in the end instead of the second pred walk, but Copilot suggested that it might be more expensive as we PHI don't have too many preds usually, but bitvectors might be long (fgMaxBlocks).

Also, I was not brave enough to just ignore PhiArgs without matching pred blocks (but if I do it, I get -200kb diff improvement).

[AndyAyersMS]

Can you describe what goes wrong without this fix?

Naively I'd expect that if we considered assertions from preds that no longer reached we would only be more conservative.

[EgorBo]

@AndyAyersMS it turned out the actual bug was in optGetEdgeAssertions that returned incorrect assertions for an edge that doesn't exist (because of RBO) for that Phi.

I propose we still keep the optVisitReachingAssertions change (it's no longer necessary to fix the bug) since the regression is pretty small just in case. We can alter the PhiVN if we want and remove dead preds somewhere (e.g. in GlobalAP)

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[EgorBo]

@AndyAyersMS anything else? Diffs are relatively small (mostly from test collections)

[AndyAyersMS]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

[EgorBo]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

@AndyAyersMS That is a very valid point. In GlobalAP when we use facts and fold comparisons into true/false, we call Morph for them that can convert BBJ_COND into BBJ_ALWAYS (e.g. via fgFoldConditional) and it currently doesn't do anything with what assertion vectors are valid. I can also take a look

[EgorBo]

/ba-g deadletter

[dhartglassMSFT]

/backport to release/10.0

[github-actions]

Started backporting to release/10.0 (link to workflow run)

[github-actions]

@dhartglassMSFT backporting to release/10.0 failed, the patch most likely resulted in conflicts. Please backport manually!

git am output

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patchApplying: ignore PhiArgs without real predsUsing index info to reconstruct a base tree...M	src/coreclr/jit/compiler.hppA	src/tests/JIT/Regression/Regression_ro_2.csprojFalling back to patching base and 3-way merge...Auto-merging src/coreclr/jit/compiler.hppCONFLICT (content): Merge conflict in src/coreclr/jit/compiler.hppCONFLICT (modify/delete): src/tests/JIT/Regression/Regression_ro_2.csproj deleted in HEAD and modified in ignore PhiArgs without real preds. Version ignore PhiArgs without real preds of src/tests/JIT/Regression/Regression_ro_2.csproj left in tree.error: Failed to merge in the changes.hint: Use 'git am --show-current-patch=diff' to see the failed patchhint: When you have resolved this problem, run "git am --continue".hint: If you prefer to skip this patch, run "git am --skip" instead.hint: To restore the original branch and stop patching, run "git am --abort".hint: Disable this message with "git config set advice.mergeConflict false"Patch failed at 0001 ignore PhiArgs without real predsError: The process '/usr/bin/git' failed with exit code 128

Link to workflow output

[EgorBo]

Fixes#124507

A couple of diffs

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT’s assertion propagation through SSA PHI nodes to be more conservative when PHI arguments don’t correspond to actual CFG predecessors, and adds a regression test for #124507.

Changes:

• Add a guard in optVisitReachingAssertions to abort PHI-based assertion inference when a PhiArg references a non-predecessor block.
• Add a new JIT regression test Runtime_124507.
• Wire the new test into Regression_ro_2.csproj.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/coreclr/jit/compiler.hpp	Abort PHI reaching-assertions walk when a PHI arg pred block is not an actual predecessor.
src/tests/JIT/Regression/Regression_ro_2.csproj	Includes the new Runtime_124507 regression test source file.
src/tests/JIT/Regression/JitBlue/Runtime_124507/Runtime_124507.cs	Adds a reduced repro as an xUnit test entry point for #124507.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[EgorBo]

PTAL @AndyAyersMS @dotnet/jit-contrib
I tried your suggestion with just checking two bitvectors in the end instead of the second pred walk, but Copilot suggested that it might be more expensive as we PHI don't have too many preds usually, but bitvectors might be long (fgMaxBlocks).

Also, I was not brave enough to just ignore PhiArgs without matching pred blocks (but if I do it, I get -200kb diff improvement).

[AndyAyersMS]

Can you describe what goes wrong without this fix?

Naively I'd expect that if we considered assertions from preds that no longer reached we would only be more conservative.

[EgorBo]

@AndyAyersMS it turned out the actual bug was in optGetEdgeAssertions that returned incorrect assertions for an edge that doesn't exist (because of RBO) for that Phi.

I propose we still keep the optVisitReachingAssertions change (it's no longer necessary to fix the bug) since the regression is pretty small just in case. We can alter the PhiVN if we want and remove dead preds somewhere (e.g. in GlobalAP)

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[EgorBo]

@AndyAyersMS anything else? Diffs are relatively small (mostly from test collections)

[AndyAyersMS]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

[EgorBo]

I have one more concern but we can address it in a follow up.

For BBJ_COND we're producing two assertion sets, one for true and one for false. It looks like the false assertion set can take advantage of knowing the JTRUE predicate is false, at least in some cases.

Now suppose something in AP modifies flow so that the BBJ_COND becomes BBJ_ALWAYS (by say resolving the JTRUE to be true and removing the false successor edge).

Now we try backwards assertion refinement like we've been increasingly doing. Successors will now look at the "false" bb assertion set from this block, but that might be wrong since it was produced assuming the predicate was false.

Seems like if we change BBJ_COND into BBJ_ALWAYS based on a true predicate, we need overwrite the "if false" assertion set with the "if true" assertion set...?

An alternative is to have the false assertion set represent the set of assertions that are true independent of the JTRUE outcome.

(similar considerations would apply in Morph with local AP, but I don't think we do any backwards refinement there)

@AndyAyersMS That is a very valid point. In GlobalAP when we use facts and fold comparisons into true/false, we call Morph for them that can convert BBJ_COND into BBJ_ALWAYS (e.g. via fgFoldConditional) and it currently doesn't do anything with what assertion vectors are valid. I can also take a look

[EgorBo]

/ba-g deadletter

[dhartglassMSFT]

/backport to release/10.0

[github-actions]

Started backporting to release/10.0 (link to workflow run)

[github-actions]

@dhartglassMSFT backporting to release/10.0 failed, the patch most likely resulted in conflicts. Please backport manually!

git am output

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patchApplying: ignore PhiArgs without real predsUsing index info to reconstruct a base tree...M	src/coreclr/jit/compiler.hppA	src/tests/JIT/Regression/Regression_ro_2.csprojFalling back to patching base and 3-way merge...Auto-merging src/coreclr/jit/compiler.hppCONFLICT (content): Merge conflict in src/coreclr/jit/compiler.hppCONFLICT (modify/delete): src/tests/JIT/Regression/Regression_ro_2.csproj deleted in HEAD and modified in ignore PhiArgs without real preds. Version ignore PhiArgs without real preds of src/tests/JIT/Regression/Regression_ro_2.csproj left in tree.error: Failed to merge in the changes.hint: Use 'git am --show-current-patch=diff' to see the failed patchhint: When you have resolved this problem, run "git am --continue".hint: If you prefer to skip this patch, run "git am --skip" instead.hint: To restore the original branch and stop patching, run "git am --abort".hint: Disable this message with "git config set advice.mergeConflict false"Patch failed at 0001 ignore PhiArgs without real predsError: The process '/usr/bin/git' failed with exit code 128

Link to workflow output