[aw0lid]

The existing equal-VN optimization for SequenceEqual is only reached when the length is not a constant. As a result, self-comparisons with a constant length are not folded to true.

Move the equal-VN check before the constant-length check so the existing optimization applies regardless of whether the length is constant.

Closes #134431

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Copilot review overview

🟡 Changes recommended

The codegen fix needs a targeted disassembly regression test.

Review effort: Balanced
Findings: 1

Open (1)

• Add HasDisasmCheck regression test for self-comparison fold · New

What changed in this PR

Moves the existing equal-value-number check so constant-length SequenceEqual self-comparisons fold to true.

Changes:

• Applies self-comparison folding before constant-length handling.
• Preserves argument side effects while eliminating redundant comparisons.

File	Description
src/​coreclr/​jit/​assertionprop.cpp	Hoists the equal-VN optimization.

[EgorBo]

LGTM. Although, I suspect there is a pre-existing bug that may fold SequenceEqual(null, null, x) as true without side-effects 🤔

[aw0lid]

@EgorBo I agree with your concern, if I understood it correctly.

With (arg1VN != ValueNumStore::NoVN) && (arg1VN == arg2VN), we fold the call to true, so the original SequenceEqual call is not executed. This means that any exception that would have been thrown inside the call is skipped.

I think we should avoid this optimization when either argument is known to be null and let the original call execute instead:

if (vnStore->IsVNNull(arg1VN) || vnStore->IsVNNull(arg2VN))
{
    return nullptr;
}

I'll verify this when I have some time.

[aw0lid]

Just a small personal note: this was my first contribution to the JIT, so seeing it merged means a lot to me. Thanks for the review!

[EgorBo]

I think we should avoid this optimization when either argument is known to be null and let the original call execute instead:

I'd just wrap 0/1 with explicit nullchecks for addresses - I'll file a PR later.

Just a small personal note: this was my first contribution to the JIT, so seeing it merged means a lot to me. Thanks for the review!

Haha, well, thanks for not trying to land a lot of changes at once like other first-time contributors try to do sometimes 🙂