arm64: Add SVE ptrue reuse pass after lowering

[jonathandavies-arm]

• Reuses equivalent ptrue producing nodes within a block via a mask temp

[dotnet-policy-service]

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

[tannergooding]

The logic here looks overall correct to me. However, diffs are not looking very good.

Rather this comes at a significant throughput hit to Arm64 (+0.54% in MinOpts) and so likely, at a minimum, needs to be skipped if optimizations are not enabled.

Then, this actually doesn't appear to be improving codegen either. Instead, it looks to significantly regress codegen (+199k bytes), particularly for MinOpts where we get a bunch of ldr, add, ldr sequences instead of ptrue or reusing an existing constant.

If you avoid running this for T0 code, then we'll still have +65k bytes of new codegen, so I imagine there is still something to be fixed/improved here. But, those diffs are generally hard to find given how much T0 code regressed and so you can't really identify what the issue is or if its the same ldr, add, ldr issue.

[tannergooding]

diffs look better now with everything but the coreclr_tests showing improvements.

Several of the tests still show regressions (+10k bytes of codegen), however, where we effectively have the following instead:

 ptrue p0.s
+ add xip1, fp, #24+ str p0, [xip1]	// [V33 rat0]
...
- ptrue p0.s+ add xip1, fp, #24+ ldr p0, [xip1]	// [V33 rat0]

Is there some particular pattern here that we're failing to account for, such as some call that is forcing a spill between the two callsites or perhaps the local being annotated in a way that forces a spill?

[tannergooding]

Changes LGTM and diffs are now purely positive.

There's still a TP hit, but I doubt that can be mitigated more as its essentially just the cost of adding a phase, whether that phase is executed or not.

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

This would notably be extendable to AllBitsSet and Zero for general SIMD or floating-point constants on all platforms (Arm64 and xarch) so isn't SVE specific and would greatly mitigate some of the issues we see in #70182 (which is a complex LSRA issue)

[jakobbotsch]

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

I would much rather see the effort spent on improving the constant reuse that LSRA already implements. This implementation seems to come with some rather severe limitations (single block only and impoverished reasoning about register kills being some of them), in addition to being a new phase that solves a problem we already try to solve.

@jonathandavies-arm Did you look into expanding LSRA's constant reuse and investigate out why some of the cases this PR handles are not handled by it?

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_reviewed_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_recorded_worker_run_id": "29684000455",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"review_id": 4730635250
}
]
}

[github-actions]

Holistic Review

Motivation: The problem is real. On arm64/SVE, predicate-producing constant masks (ptrue/pfalse) are frequently rematerialized multiple times within a block, and the JIT had no post-lowering mechanism to share an equivalent predicate via a temp. Reducing redundant ptrue/pfalse materialization is a legitimate codegen win.

Approach: The approach is sound and appropriately conservative. It introduces a new post-lowering PHASE_CONSTANT_REUSE (arm64 + FEATURE_MASKED_HW_INTRINSICS, optimizations only), materializing the first equivalent constant-mask into a local temp and rewriting later equivalent uses as loads, relying on normal local-var lifetimes so LSRA models predicate clobbers. The refactor that hoists the post-lowering liveness/ref-count/dead-block cleanup out of Lowering::DoPhase into a new Compiler::fgPostLowering phase is a clean mechanical move that preserves the original ordering and semantics, and correctly runs after the new reuse phase. The guards are careful: it bails when locals are not enregistered, skips all-true masks whose live range crosses a call, respects contained masks by poisoning the whole group, and preserves the ConditionalSelect(AllTrue, embedded-op, zero) movprfx-free codegen special case. Pattern normalization (LargestPowerOf2 -> All, pfalse sentinel keyed as .b) matches how these lower.

Summary: ✅ LGTM. The change is well-structured, guarded conservatively, and already carries an approval from a JIT maintainer (tannergooding). The refactor preserves prior post-lowering semantics, and the added FileCheck-based test covers a good spread of true/false/pattern/embedded/conversion/SVE2 cases plus single-vs-multiple reuse scenarios. One minor non-blocking observation: the new IsSveBreakMask helper in hwintrinsic.h is dead code with no callers (flagged inline). Nothing here blocks merge.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 146.3 AIC · ⌖ 10.5 AIC · ⊞ 10K

[github-actions]

💡 IsSveBreakMask is added here but never referenced anywhere in the JIT (IsSveCreateTrueMask above it is used by constantreuse.cpp, but this helper has no callers). If it is intended for a follow-up, a brief comment noting that would help; otherwise consider dropping it to avoid dead code.

[jonathandavies-arm]

I'm going to close this PR and the LSRA version of the work is here #131309.

[jonathandavies-arm]

• Reuses equivalent ptrue producing nodes within a block via a mask temp

[dotnet-policy-service]

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

[tannergooding]

The logic here looks overall correct to me. However, diffs are not looking very good.

Rather this comes at a significant throughput hit to Arm64 (+0.54% in MinOpts) and so likely, at a minimum, needs to be skipped if optimizations are not enabled.

Then, this actually doesn't appear to be improving codegen either. Instead, it looks to significantly regress codegen (+199k bytes), particularly for MinOpts where we get a bunch of ldr, add, ldr sequences instead of ptrue or reusing an existing constant.

If you avoid running this for T0 code, then we'll still have +65k bytes of new codegen, so I imagine there is still something to be fixed/improved here. But, those diffs are generally hard to find given how much T0 code regressed and so you can't really identify what the issue is or if its the same ldr, add, ldr issue.

[tannergooding]

diffs look better now with everything but the coreclr_tests showing improvements.

Several of the tests still show regressions (+10k bytes of codegen), however, where we effectively have the following instead:

 ptrue p0.s
+ add xip1, fp, #24+ str p0, [xip1]	// [V33 rat0]
...
- ptrue p0.s+ add xip1, fp, #24+ ldr p0, [xip1]	// [V33 rat0]

Is there some particular pattern here that we're failing to account for, such as some call that is forcing a spill between the two callsites or perhaps the local being annotated in a way that forces a spill?

[tannergooding]

Changes LGTM and diffs are now purely positive.

There's still a TP hit, but I doubt that can be mitigated more as its essentially just the cost of adding a phase, whether that phase is executed or not.

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

This would notably be extendable to AllBitsSet and Zero for general SIMD or floating-point constants on all platforms (Arm64 and xarch) so isn't SVE specific and would greatly mitigate some of the issues we see in #70182 (which is a complex LSRA issue)

[jakobbotsch]

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

I would much rather see the effort spent on improving the constant reuse that LSRA already implements. This implementation seems to come with some rather severe limitations (single block only and impoverished reasoning about register kills being some of them), in addition to being a new phase that solves a problem we already try to solve.

@jonathandavies-arm Did you look into expanding LSRA's constant reuse and investigate out why some of the cases this PR handles are not handled by it?

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_reviewed_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_recorded_worker_run_id": "29684000455",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"review_id": 4730635250
}
]
}

[github-actions]

Holistic Review

Motivation: The problem is real. On arm64/SVE, predicate-producing constant masks (ptrue/pfalse) are frequently rematerialized multiple times within a block, and the JIT had no post-lowering mechanism to share an equivalent predicate via a temp. Reducing redundant ptrue/pfalse materialization is a legitimate codegen win.

Approach: The approach is sound and appropriately conservative. It introduces a new post-lowering PHASE_CONSTANT_REUSE (arm64 + FEATURE_MASKED_HW_INTRINSICS, optimizations only), materializing the first equivalent constant-mask into a local temp and rewriting later equivalent uses as loads, relying on normal local-var lifetimes so LSRA models predicate clobbers. The refactor that hoists the post-lowering liveness/ref-count/dead-block cleanup out of Lowering::DoPhase into a new Compiler::fgPostLowering phase is a clean mechanical move that preserves the original ordering and semantics, and correctly runs after the new reuse phase. The guards are careful: it bails when locals are not enregistered, skips all-true masks whose live range crosses a call, respects contained masks by poisoning the whole group, and preserves the ConditionalSelect(AllTrue, embedded-op, zero) movprfx-free codegen special case. Pattern normalization (LargestPowerOf2 -> All, pfalse sentinel keyed as .b) matches how these lower.

Summary: ✅ LGTM. The change is well-structured, guarded conservatively, and already carries an approval from a JIT maintainer (tannergooding). The refactor preserves prior post-lowering semantics, and the added FileCheck-based test covers a good spread of true/false/pattern/embedded/conversion/SVE2 cases plus single-vs-multiple reuse scenarios. One minor non-blocking observation: the new IsSveBreakMask helper in hwintrinsic.h is dead code with no callers (flagged inline). Nothing here blocks merge.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 146.3 AIC · ⌖ 10.5 AIC · ⊞ 10K

[github-actions]

💡 IsSveBreakMask is added here but never referenced anywhere in the JIT (IsSveCreateTrueMask above it is used by constantreuse.cpp, but this helper has no callers). If it is intended for a follow-up, a brief comment noting that would help; otherwise consider dropping it to avoid dead code.

[jonathandavies-arm]

I'm going to close this PR and the LSRA version of the work is here #131309.

[jonathandavies-arm]

• Reuses equivalent ptrue producing nodes within a block via a mask temp

[dotnet-policy-service]

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

[tannergooding]

The logic here looks overall correct to me. However, diffs are not looking very good.

Rather this comes at a significant throughput hit to Arm64 (+0.54% in MinOpts) and so likely, at a minimum, needs to be skipped if optimizations are not enabled.

Then, this actually doesn't appear to be improving codegen either. Instead, it looks to significantly regress codegen (+199k bytes), particularly for MinOpts where we get a bunch of ldr, add, ldr sequences instead of ptrue or reusing an existing constant.

If you avoid running this for T0 code, then we'll still have +65k bytes of new codegen, so I imagine there is still something to be fixed/improved here. But, those diffs are generally hard to find given how much T0 code regressed and so you can't really identify what the issue is or if its the same ldr, add, ldr issue.

[tannergooding]

diffs look better now with everything but the coreclr_tests showing improvements.

Several of the tests still show regressions (+10k bytes of codegen), however, where we effectively have the following instead:

 ptrue p0.s
+ add xip1, fp, #24+ str p0, [xip1]	// [V33 rat0]
...
- ptrue p0.s+ add xip1, fp, #24+ ldr p0, [xip1]	// [V33 rat0]

Is there some particular pattern here that we're failing to account for, such as some call that is forcing a spill between the two callsites or perhaps the local being annotated in a way that forces a spill?

[tannergooding]

Changes LGTM and diffs are now purely positive.

There's still a TP hit, but I doubt that can be mitigated more as its essentially just the cost of adding a phase, whether that phase is executed or not.

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

This would notably be extendable to AllBitsSet and Zero for general SIMD or floating-point constants on all platforms (Arm64 and xarch) so isn't SVE specific and would greatly mitigate some of the issues we see in #70182 (which is a complex LSRA issue)

[jakobbotsch]

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

I would much rather see the effort spent on improving the constant reuse that LSRA already implements. This implementation seems to come with some rather severe limitations (single block only and impoverished reasoning about register kills being some of them), in addition to being a new phase that solves a problem we already try to solve.

@jonathandavies-arm Did you look into expanding LSRA's constant reuse and investigate out why some of the cases this PR handles are not handled by it?

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_reviewed_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_recorded_worker_run_id": "29684000455",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"review_id": 4730635250
}
]
}

[github-actions]

Holistic Review

Motivation: The problem is real. On arm64/SVE, predicate-producing constant masks (ptrue/pfalse) are frequently rematerialized multiple times within a block, and the JIT had no post-lowering mechanism to share an equivalent predicate via a temp. Reducing redundant ptrue/pfalse materialization is a legitimate codegen win.

Approach: The approach is sound and appropriately conservative. It introduces a new post-lowering PHASE_CONSTANT_REUSE (arm64 + FEATURE_MASKED_HW_INTRINSICS, optimizations only), materializing the first equivalent constant-mask into a local temp and rewriting later equivalent uses as loads, relying on normal local-var lifetimes so LSRA models predicate clobbers. The refactor that hoists the post-lowering liveness/ref-count/dead-block cleanup out of Lowering::DoPhase into a new Compiler::fgPostLowering phase is a clean mechanical move that preserves the original ordering and semantics, and correctly runs after the new reuse phase. The guards are careful: it bails when locals are not enregistered, skips all-true masks whose live range crosses a call, respects contained masks by poisoning the whole group, and preserves the ConditionalSelect(AllTrue, embedded-op, zero) movprfx-free codegen special case. Pattern normalization (LargestPowerOf2 -> All, pfalse sentinel keyed as .b) matches how these lower.

Summary: ✅ LGTM. The change is well-structured, guarded conservatively, and already carries an approval from a JIT maintainer (tannergooding). The refactor preserves prior post-lowering semantics, and the added FileCheck-based test covers a good spread of true/false/pattern/embedded/conversion/SVE2 cases plus single-vs-multiple reuse scenarios. One minor non-blocking observation: the new IsSveBreakMask helper in hwintrinsic.h is dead code with no callers (flagged inline). Nothing here blocks merge.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 146.3 AIC · ⌖ 10.5 AIC · ⊞ 10K

[github-actions]

💡 IsSveBreakMask is added here but never referenced anywhere in the JIT (IsSveCreateTrueMask above it is used by constantreuse.cpp, but this helper has no callers). If it is intended for a follow-up, a brief comment noting that would help; otherwise consider dropping it to avoid dead code.

[jonathandavies-arm]

I'm going to close this PR and the LSRA version of the work is here #131309.

[jonathandavies-arm]

• Reuses equivalent ptrue producing nodes within a block via a mask temp

[dotnet-policy-service]

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

[tannergooding]

The logic here looks overall correct to me. However, diffs are not looking very good.

Rather this comes at a significant throughput hit to Arm64 (+0.54% in MinOpts) and so likely, at a minimum, needs to be skipped if optimizations are not enabled.

Then, this actually doesn't appear to be improving codegen either. Instead, it looks to significantly regress codegen (+199k bytes), particularly for MinOpts where we get a bunch of ldr, add, ldr sequences instead of ptrue or reusing an existing constant.

If you avoid running this for T0 code, then we'll still have +65k bytes of new codegen, so I imagine there is still something to be fixed/improved here. But, those diffs are generally hard to find given how much T0 code regressed and so you can't really identify what the issue is or if its the same ldr, add, ldr issue.

[tannergooding]

diffs look better now with everything but the coreclr_tests showing improvements.

Several of the tests still show regressions (+10k bytes of codegen), however, where we effectively have the following instead:

 ptrue p0.s
+ add xip1, fp, #24+ str p0, [xip1]	// [V33 rat0]
...
- ptrue p0.s+ add xip1, fp, #24+ ldr p0, [xip1]	// [V33 rat0]

Is there some particular pattern here that we're failing to account for, such as some call that is forcing a spill between the two callsites or perhaps the local being annotated in a way that forces a spill?

[tannergooding]

Changes LGTM and diffs are now purely positive.

There's still a TP hit, but I doubt that can be mitigated more as its essentially just the cost of adding a phase, whether that phase is executed or not.

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

This would notably be extendable to AllBitsSet and Zero for general SIMD or floating-point constants on all platforms (Arm64 and xarch) so isn't SVE specific and would greatly mitigate some of the issues we see in #70182 (which is a complex LSRA issue)

[jakobbotsch]

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

I would much rather see the effort spent on improving the constant reuse that LSRA already implements. This implementation seems to come with some rather severe limitations (single block only and impoverished reasoning about register kills being some of them), in addition to being a new phase that solves a problem we already try to solve.

@jonathandavies-arm Did you look into expanding LSRA's constant reuse and investigate out why some of the cases this PR handles are not handled by it?

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_reviewed_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_recorded_worker_run_id": "29684000455",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"review_id": 4730635250
}
]
}

[github-actions]

Holistic Review

Motivation: The problem is real. On arm64/SVE, predicate-producing constant masks (ptrue/pfalse) are frequently rematerialized multiple times within a block, and the JIT had no post-lowering mechanism to share an equivalent predicate via a temp. Reducing redundant ptrue/pfalse materialization is a legitimate codegen win.

Approach: The approach is sound and appropriately conservative. It introduces a new post-lowering PHASE_CONSTANT_REUSE (arm64 + FEATURE_MASKED_HW_INTRINSICS, optimizations only), materializing the first equivalent constant-mask into a local temp and rewriting later equivalent uses as loads, relying on normal local-var lifetimes so LSRA models predicate clobbers. The refactor that hoists the post-lowering liveness/ref-count/dead-block cleanup out of Lowering::DoPhase into a new Compiler::fgPostLowering phase is a clean mechanical move that preserves the original ordering and semantics, and correctly runs after the new reuse phase. The guards are careful: it bails when locals are not enregistered, skips all-true masks whose live range crosses a call, respects contained masks by poisoning the whole group, and preserves the ConditionalSelect(AllTrue, embedded-op, zero) movprfx-free codegen special case. Pattern normalization (LargestPowerOf2 -> All, pfalse sentinel keyed as .b) matches how these lower.

Summary: ✅ LGTM. The change is well-structured, guarded conservatively, and already carries an approval from a JIT maintainer (tannergooding). The refactor preserves prior post-lowering semantics, and the added FileCheck-based test covers a good spread of true/false/pattern/embedded/conversion/SVE2 cases plus single-vs-multiple reuse scenarios. One minor non-blocking observation: the new IsSveBreakMask helper in hwintrinsic.h is dead code with no callers (flagged inline). Nothing here blocks merge.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 146.3 AIC · ⌖ 10.5 AIC · ⊞ 10K

[github-actions]

💡 IsSveBreakMask is added here but never referenced anywhere in the JIT (IsSveCreateTrueMask above it is used by constantreuse.cpp, but this helper has no callers). If it is intended for a follow-up, a brief comment noting that would help; otherwise consider dropping it to avoid dead code.

[jonathandavies-arm]

I'm going to close this PR and the LSRA version of the work is here #131309.

[jonathandavies-arm]

• Reuses equivalent ptrue producing nodes within a block via a mask temp

[dotnet-policy-service]

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

[tannergooding]

The logic here looks overall correct to me. However, diffs are not looking very good.

Rather this comes at a significant throughput hit to Arm64 (+0.54% in MinOpts) and so likely, at a minimum, needs to be skipped if optimizations are not enabled.

Then, this actually doesn't appear to be improving codegen either. Instead, it looks to significantly regress codegen (+199k bytes), particularly for MinOpts where we get a bunch of ldr, add, ldr sequences instead of ptrue or reusing an existing constant.

If you avoid running this for T0 code, then we'll still have +65k bytes of new codegen, so I imagine there is still something to be fixed/improved here. But, those diffs are generally hard to find given how much T0 code regressed and so you can't really identify what the issue is or if its the same ldr, add, ldr issue.

[tannergooding]

diffs look better now with everything but the coreclr_tests showing improvements.

Several of the tests still show regressions (+10k bytes of codegen), however, where we effectively have the following instead:

 ptrue p0.s
+ add xip1, fp, #24+ str p0, [xip1]	// [V33 rat0]
...
- ptrue p0.s+ add xip1, fp, #24+ ldr p0, [xip1]	// [V33 rat0]

Is there some particular pattern here that we're failing to account for, such as some call that is forcing a spill between the two callsites or perhaps the local being annotated in a way that forces a spill?

[tannergooding]

Changes LGTM and diffs are now purely positive.

There's still a TP hit, but I doubt that can be mitigated more as its essentially just the cost of adding a phase, whether that phase is executed or not.

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

This would notably be extendable to AllBitsSet and Zero for general SIMD or floating-point constants on all platforms (Arm64 and xarch) so isn't SVE specific and would greatly mitigate some of the issues we see in #70182 (which is a complex LSRA issue)

[jakobbotsch]

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

I would much rather see the effort spent on improving the constant reuse that LSRA already implements. This implementation seems to come with some rather severe limitations (single block only and impoverished reasoning about register kills being some of them), in addition to being a new phase that solves a problem we already try to solve.

@jonathandavies-arm Did you look into expanding LSRA's constant reuse and investigate out why some of the cases this PR handles are not handled by it?

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_reviewed_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_recorded_worker_run_id": "29684000455",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"review_id": 4730635250
}
]
}

[github-actions]

Holistic Review

Motivation: The problem is real. On arm64/SVE, predicate-producing constant masks (ptrue/pfalse) are frequently rematerialized multiple times within a block, and the JIT had no post-lowering mechanism to share an equivalent predicate via a temp. Reducing redundant ptrue/pfalse materialization is a legitimate codegen win.

Approach: The approach is sound and appropriately conservative. It introduces a new post-lowering PHASE_CONSTANT_REUSE (arm64 + FEATURE_MASKED_HW_INTRINSICS, optimizations only), materializing the first equivalent constant-mask into a local temp and rewriting later equivalent uses as loads, relying on normal local-var lifetimes so LSRA models predicate clobbers. The refactor that hoists the post-lowering liveness/ref-count/dead-block cleanup out of Lowering::DoPhase into a new Compiler::fgPostLowering phase is a clean mechanical move that preserves the original ordering and semantics, and correctly runs after the new reuse phase. The guards are careful: it bails when locals are not enregistered, skips all-true masks whose live range crosses a call, respects contained masks by poisoning the whole group, and preserves the ConditionalSelect(AllTrue, embedded-op, zero) movprfx-free codegen special case. Pattern normalization (LargestPowerOf2 -> All, pfalse sentinel keyed as .b) matches how these lower.

Summary: ✅ LGTM. The change is well-structured, guarded conservatively, and already carries an approval from a JIT maintainer (tannergooding). The refactor preserves prior post-lowering semantics, and the added FileCheck-based test covers a good spread of true/false/pattern/embedded/conversion/SVE2 cases plus single-vs-multiple reuse scenarios. One minor non-blocking observation: the new IsSveBreakMask helper in hwintrinsic.h is dead code with no callers (flagged inline). Nothing here blocks merge.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 146.3 AIC · ⌖ 10.5 AIC · ⊞ 10K

[github-actions]

💡 IsSveBreakMask is added here but never referenced anywhere in the JIT (IsSveCreateTrueMask above it is used by constantreuse.cpp, but this helper has no callers). If it is intended for a follow-up, a brief comment noting that would help; otherwise consider dropping it to avoid dead code.

[jonathandavies-arm]

I'm going to close this PR and the LSRA version of the work is here #131309.

[jonathandavies-arm]

• Reuses equivalent ptrue producing nodes within a block via a mask temp

[dotnet-policy-service]

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

[tannergooding]

The logic here looks overall correct to me. However, diffs are not looking very good.

Rather this comes at a significant throughput hit to Arm64 (+0.54% in MinOpts) and so likely, at a minimum, needs to be skipped if optimizations are not enabled.

Then, this actually doesn't appear to be improving codegen either. Instead, it looks to significantly regress codegen (+199k bytes), particularly for MinOpts where we get a bunch of ldr, add, ldr sequences instead of ptrue or reusing an existing constant.

If you avoid running this for T0 code, then we'll still have +65k bytes of new codegen, so I imagine there is still something to be fixed/improved here. But, those diffs are generally hard to find given how much T0 code regressed and so you can't really identify what the issue is or if its the same ldr, add, ldr issue.

[tannergooding]

diffs look better now with everything but the coreclr_tests showing improvements.

Several of the tests still show regressions (+10k bytes of codegen), however, where we effectively have the following instead:

 ptrue p0.s
+ add xip1, fp, #24+ str p0, [xip1]	// [V33 rat0]
...
- ptrue p0.s+ add xip1, fp, #24+ ldr p0, [xip1]	// [V33 rat0]

Is there some particular pattern here that we're failing to account for, such as some call that is forcing a spill between the two callsites or perhaps the local being annotated in a way that forces a spill?

[tannergooding]

Changes LGTM and diffs are now purely positive.

There's still a TP hit, but I doubt that can be mitigated more as its essentially just the cost of adding a phase, whether that phase is executed or not.

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

This would notably be extendable to AllBitsSet and Zero for general SIMD or floating-point constants on all platforms (Arm64 and xarch) so isn't SVE specific and would greatly mitigate some of the issues we see in #70182 (which is a complex LSRA issue)

[jakobbotsch]

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

I would much rather see the effort spent on improving the constant reuse that LSRA already implements. This implementation seems to come with some rather severe limitations (single block only and impoverished reasoning about register kills being some of them), in addition to being a new phase that solves a problem we already try to solve.

@jonathandavies-arm Did you look into expanding LSRA's constant reuse and investigate out why some of the cases this PR handles are not handled by it?

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_reviewed_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_recorded_worker_run_id": "29684000455",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"review_id": 4730635250
}
]
}

[github-actions]

Holistic Review

Motivation: The problem is real. On arm64/SVE, predicate-producing constant masks (ptrue/pfalse) are frequently rematerialized multiple times within a block, and the JIT had no post-lowering mechanism to share an equivalent predicate via a temp. Reducing redundant ptrue/pfalse materialization is a legitimate codegen win.

Approach: The approach is sound and appropriately conservative. It introduces a new post-lowering PHASE_CONSTANT_REUSE (arm64 + FEATURE_MASKED_HW_INTRINSICS, optimizations only), materializing the first equivalent constant-mask into a local temp and rewriting later equivalent uses as loads, relying on normal local-var lifetimes so LSRA models predicate clobbers. The refactor that hoists the post-lowering liveness/ref-count/dead-block cleanup out of Lowering::DoPhase into a new Compiler::fgPostLowering phase is a clean mechanical move that preserves the original ordering and semantics, and correctly runs after the new reuse phase. The guards are careful: it bails when locals are not enregistered, skips all-true masks whose live range crosses a call, respects contained masks by poisoning the whole group, and preserves the ConditionalSelect(AllTrue, embedded-op, zero) movprfx-free codegen special case. Pattern normalization (LargestPowerOf2 -> All, pfalse sentinel keyed as .b) matches how these lower.

Summary: ✅ LGTM. The change is well-structured, guarded conservatively, and already carries an approval from a JIT maintainer (tannergooding). The refactor preserves prior post-lowering semantics, and the added FileCheck-based test covers a good spread of true/false/pattern/embedded/conversion/SVE2 cases plus single-vs-multiple reuse scenarios. One minor non-blocking observation: the new IsSveBreakMask helper in hwintrinsic.h is dead code with no callers (flagged inline). Nothing here blocks merge.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 146.3 AIC · ⌖ 10.5 AIC · ⊞ 10K

[github-actions]

💡 IsSveBreakMask is added here but never referenced anywhere in the JIT (IsSveCreateTrueMask above it is used by constantreuse.cpp, but this helper has no callers). If it is intended for a follow-up, a brief comment noting that would help; otherwise consider dropping it to avoid dead code.

[jonathandavies-arm]

I'm going to close this PR and the LSRA version of the work is here #131309.

[jonathandavies-arm]

• Reuses equivalent ptrue producing nodes within a block via a mask temp

[dotnet-policy-service]

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

[tannergooding]

The logic here looks overall correct to me. However, diffs are not looking very good.

Rather this comes at a significant throughput hit to Arm64 (+0.54% in MinOpts) and so likely, at a minimum, needs to be skipped if optimizations are not enabled.

Then, this actually doesn't appear to be improving codegen either. Instead, it looks to significantly regress codegen (+199k bytes), particularly for MinOpts where we get a bunch of ldr, add, ldr sequences instead of ptrue or reusing an existing constant.

If you avoid running this for T0 code, then we'll still have +65k bytes of new codegen, so I imagine there is still something to be fixed/improved here. But, those diffs are generally hard to find given how much T0 code regressed and so you can't really identify what the issue is or if its the same ldr, add, ldr issue.

[tannergooding]

diffs look better now with everything but the coreclr_tests showing improvements.

Several of the tests still show regressions (+10k bytes of codegen), however, where we effectively have the following instead:

 ptrue p0.s
+ add xip1, fp, #24+ str p0, [xip1]	// [V33 rat0]
...
- ptrue p0.s+ add xip1, fp, #24+ ldr p0, [xip1]	// [V33 rat0]

Is there some particular pattern here that we're failing to account for, such as some call that is forcing a spill between the two callsites or perhaps the local being annotated in a way that forces a spill?

[tannergooding]

Changes LGTM and diffs are now purely positive.

There's still a TP hit, but I doubt that can be mitigated more as its essentially just the cost of adding a phase, whether that phase is executed or not.

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

This would notably be extendable to AllBitsSet and Zero for general SIMD or floating-point constants on all platforms (Arm64 and xarch) so isn't SVE specific and would greatly mitigate some of the issues we see in #70182 (which is a complex LSRA issue)

[jakobbotsch]

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

I would much rather see the effort spent on improving the constant reuse that LSRA already implements. This implementation seems to come with some rather severe limitations (single block only and impoverished reasoning about register kills being some of them), in addition to being a new phase that solves a problem we already try to solve.

@jonathandavies-arm Did you look into expanding LSRA's constant reuse and investigate out why some of the cases this PR handles are not handled by it?

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_reviewed_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_recorded_worker_run_id": "29684000455",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"review_id": 4730635250
}
]
}

[github-actions]

Holistic Review

Motivation: The problem is real. On arm64/SVE, predicate-producing constant masks (ptrue/pfalse) are frequently rematerialized multiple times within a block, and the JIT had no post-lowering mechanism to share an equivalent predicate via a temp. Reducing redundant ptrue/pfalse materialization is a legitimate codegen win.

Approach: The approach is sound and appropriately conservative. It introduces a new post-lowering PHASE_CONSTANT_REUSE (arm64 + FEATURE_MASKED_HW_INTRINSICS, optimizations only), materializing the first equivalent constant-mask into a local temp and rewriting later equivalent uses as loads, relying on normal local-var lifetimes so LSRA models predicate clobbers. The refactor that hoists the post-lowering liveness/ref-count/dead-block cleanup out of Lowering::DoPhase into a new Compiler::fgPostLowering phase is a clean mechanical move that preserves the original ordering and semantics, and correctly runs after the new reuse phase. The guards are careful: it bails when locals are not enregistered, skips all-true masks whose live range crosses a call, respects contained masks by poisoning the whole group, and preserves the ConditionalSelect(AllTrue, embedded-op, zero) movprfx-free codegen special case. Pattern normalization (LargestPowerOf2 -> All, pfalse sentinel keyed as .b) matches how these lower.

Summary: ✅ LGTM. The change is well-structured, guarded conservatively, and already carries an approval from a JIT maintainer (tannergooding). The refactor preserves prior post-lowering semantics, and the added FileCheck-based test covers a good spread of true/false/pattern/embedded/conversion/SVE2 cases plus single-vs-multiple reuse scenarios. One minor non-blocking observation: the new IsSveBreakMask helper in hwintrinsic.h is dead code with no callers (flagged inline). Nothing here blocks merge.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 146.3 AIC · ⌖ 10.5 AIC · ⊞ 10K

[github-actions]

💡 IsSveBreakMask is added here but never referenced anywhere in the JIT (IsSveCreateTrueMask above it is used by constantreuse.cpp, but this helper has no callers). If it is intended for a follow-up, a brief comment noting that would help; otherwise consider dropping it to avoid dead code.

[jonathandavies-arm]

I'm going to close this PR and the LSRA version of the work is here #131309.

[jonathandavies-arm]

• Reuses equivalent ptrue producing nodes within a block via a mask temp

[dotnet-policy-service]

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

[tannergooding]

The logic here looks overall correct to me. However, diffs are not looking very good.

Rather this comes at a significant throughput hit to Arm64 (+0.54% in MinOpts) and so likely, at a minimum, needs to be skipped if optimizations are not enabled.

Then, this actually doesn't appear to be improving codegen either. Instead, it looks to significantly regress codegen (+199k bytes), particularly for MinOpts where we get a bunch of ldr, add, ldr sequences instead of ptrue or reusing an existing constant.

If you avoid running this for T0 code, then we'll still have +65k bytes of new codegen, so I imagine there is still something to be fixed/improved here. But, those diffs are generally hard to find given how much T0 code regressed and so you can't really identify what the issue is or if its the same ldr, add, ldr issue.

[tannergooding]

diffs look better now with everything but the coreclr_tests showing improvements.

Several of the tests still show regressions (+10k bytes of codegen), however, where we effectively have the following instead:

 ptrue p0.s
+ add xip1, fp, #24+ str p0, [xip1]	// [V33 rat0]
...
- ptrue p0.s+ add xip1, fp, #24+ ldr p0, [xip1]	// [V33 rat0]

Is there some particular pattern here that we're failing to account for, such as some call that is forcing a spill between the two callsites or perhaps the local being annotated in a way that forces a spill?

[tannergooding]

Changes LGTM and diffs are now purely positive.

There's still a TP hit, but I doubt that can be mitigated more as its essentially just the cost of adding a phase, whether that phase is executed or not.

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

This would notably be extendable to AllBitsSet and Zero for general SIMD or floating-point constants on all platforms (Arm64 and xarch) so isn't SVE specific and would greatly mitigate some of the issues we see in #70182 (which is a complex LSRA issue)

[jakobbotsch]

@dotnet/jit-contrib, @jakobbotsch, @EgorBo for secondary review and input on whether such a phase is worth having.

I would much rather see the effort spent on improving the constant reuse that LSRA already implements. This implementation seems to come with some rather severe limitations (single block only and impoverished reasoning about register kills being some of them), in addition to being a new phase that solves a problem we already try to solve.

@jonathandavies-arm Did you look into expanding LSRA's constant reuse and investigate out why some of the cases this PR handles are not handled by it?

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_reviewed_commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e137b2c018221d7babd79162f0fd151367c34c58",
"last_recorded_worker_run_id": "29684000455",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "8d3f5534cb1ee5dba6f1fd30e30fc8c97674f322",
"review_id": 4730635250
}
]
}

[github-actions]

Holistic Review

Motivation: The problem is real. On arm64/SVE, predicate-producing constant masks (ptrue/pfalse) are frequently rematerialized multiple times within a block, and the JIT had no post-lowering mechanism to share an equivalent predicate via a temp. Reducing redundant ptrue/pfalse materialization is a legitimate codegen win.

Approach: The approach is sound and appropriately conservative. It introduces a new post-lowering PHASE_CONSTANT_REUSE (arm64 + FEATURE_MASKED_HW_INTRINSICS, optimizations only), materializing the first equivalent constant-mask into a local temp and rewriting later equivalent uses as loads, relying on normal local-var lifetimes so LSRA models predicate clobbers. The refactor that hoists the post-lowering liveness/ref-count/dead-block cleanup out of Lowering::DoPhase into a new Compiler::fgPostLowering phase is a clean mechanical move that preserves the original ordering and semantics, and correctly runs after the new reuse phase. The guards are careful: it bails when locals are not enregistered, skips all-true masks whose live range crosses a call, respects contained masks by poisoning the whole group, and preserves the ConditionalSelect(AllTrue, embedded-op, zero) movprfx-free codegen special case. Pattern normalization (LargestPowerOf2 -> All, pfalse sentinel keyed as .b) matches how these lower.

Summary: ✅ LGTM. The change is well-structured, guarded conservatively, and already carries an approval from a JIT maintainer (tannergooding). The refactor preserves prior post-lowering semantics, and the added FileCheck-based test covers a good spread of true/false/pattern/embedded/conversion/SVE2 cases plus single-vs-multiple reuse scenarios. One minor non-blocking observation: the new IsSveBreakMask helper in hwintrinsic.h is dead code with no callers (flagged inline). Nothing here blocks merge.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 146.3 AIC · ⌖ 10.5 AIC · ⊞ 10K

[github-actions]

💡 IsSveBreakMask is added here but never referenced anywhere in the JIT (IsSveCreateTrueMask above it is used by constantreuse.cpp, but this helper has no callers). If it is intended for a follow-up, a brief comment noting that would help; otherwise consider dropping it to avoid dead code.

[jonathandavies-arm]

I'm going to close this PR and the LSRA version of the work is here #131309.