JIT: refine SSA info in ambiguously threaded blocks

[AndyAyersMS]

If we partially jump-thread a block with PHIs, the remaining preds that target block may all bring in the same SSA def for some of the PHIs. If so, remove the PHIs and update the SSA/VN for the PHI uses in the block.

In particular there may just be one ambiguous pred.

Also: enhance AP slightly to handle non-negative cases compared to negative values.

Fixes #126703.

[AndyAyersMS]

@EgorBo PTAL
fyi @dotnet/jit-contrib

[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

Refines CoreCLR JIT optimizations around jump-threading through PHI-containing blocks by improving SSA/VN rewriting when only ambiguously-threaded predecessors remain, and adds a small assertion propagation enhancement to fold comparisons against negative constants when the compared value is proven non-negative.

Changes:

• Track and remove redundant PHI-def statements in partially jump-threaded blocks when remaining incoming paths agree on the same SSA def, and rewrite SSA/VN for affected uses.
• Adjust successor SSA replacement computation to reflect post-threading reaching paths.
• Enhance assertion propagation to fold ==/!= against negative constants when the value is known non-negative; add a regression test with disasm CHECKs.

Reviewed changes

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

File	Description
src/tests/JIT/opt/RedundantBranch/JumpThreadPhi.cs	Adds disasm CHECK expectations to validate improved codegen for the repro pattern.
src/coreclr/jit/redundantbranchopts.cpp	Implements PHI-use rewriting + optional PHI removal when ambiguous preds converge on a single SSA def after partial threading.
src/coreclr/jit/assertionprop.cpp	Adds folding for X ==/!= (negative const) when X is proven never-negative.

[Copilot]

The new disasm CHECK patterns are very xarch- and register-specific (e.g., tzcnt, cmp eax, 32, cmove). If these CHECKs are evaluated on non-xarch targets, or if register allocation/codegen changes (e.g., using a different register than eax), this test may become flaky/fail without a regression in the optimization. Consider making the CHECKs architecture-neutral (e.g., checking for a semantic helper/pattern rather than exact x86 mnemonics/registers), or otherwise constraining the CHECKs so they only run for the intended target(s).

Suggested change

	// CHECK: cmp eax, 32
	// CHECK-NOT: cmove
	// CHECK-NOT: cmp eax, -1
	// CHECK: cmp{{.*}}, 32
	// CHECK-NOT: cmov
	// CHECK-NOT: cmp{{.*}}, -1

[AndyAyersMS]

From the inspiring example

	Control flow and IR before RBO. We are going to jump thread through BB4, but it's only partial threading since we don't know how BB4's predicate evaluates when control comes from BB3.
	Partial jump threading is done. This is how things are left in main. But with this PR we now realize that there is just one reaching def for V3 in BB4, and so we rewrite the use and remove the PHI
	Assertion prop is then able to propagate the "never negative" range and eliminate the compare in BB4.
	Post-AP there is just one compare left...

[EgorBo]

Pretty sure it should be handled by

https://github.com/AndyAyersMS/runtime/blob/0a30b5d5b82cce32aef58b53d41b2f349bf7c452/src/coreclr/jit/assertionprop.cpp#L4291-L4302

below this code. If it doesn't we need to check why, I can quickly check

[AndyAyersMS]

We might also be able to evaluate better in VN? Perhaps that would be a simpler or complementary change (we could then fully jump thread).

Eg VN should know that (EQ (never-negative V3.2), -1) is always false.

[EgorBo]

We might also be able to evaluate better in VN?

I am not sure we can, in your case (I've checked locally) we know that the left side is never negative due to X >= 1 assertion (I analyzed a random SPMI diff generated by this change specifically)

So I've checked locally - this change does generate a couple of diffs, but mainly because for some reason valueTreeVN = optConservativeNormalVN(valueTree) returns some other VN than

    auto relopVN = optConservativeNormalVN(tree);
    VNFuncApp funcApp;
    bool found = vnStore->GetVNFunc(relopVN, &funcApp);
    auto relopOp1VN = funcApp.m_args[0];
    auto relopOp2VN = funcApp.m_args[1];

I'm not sure why it doesn't match. Both valueTreeVN and relopOp1VN are normalized VN MemOpaque:NotInLoop

[EgorBo]

I propose we don't check in this change and I'll investigate why VN don't match. Because this effectively duplicate the logic inside GetRangeFromAssertions for relops.
Ideally, GetRangeFromAssertions should recursively call itself for op1 and op2 for a relop, and see if it can evaluate the relations between two ranges e.g. [1..int.Max] != [-1..-1] -> always true.

[EgorBo]

Yeah, so it seems some optimization physically changed trees in a relop. So now VNFuncApp's op1 and op2 for that relop don't match actual VNs for tree->gtOp1 and gtOp2. I can workaround it

[AndyAyersMS]

RBO can change the VN on the compare, perhaps it's the culprit?

runtime/src/coreclr/jit/redundantbranchopts.cpp

Lines 2430 to 2456 in e697a30

	// If block didn't get fully optimized, and now has just one pred, see if
	// we can sharpen the predicate's VN.
	//
	// (Todo, perhaps: revisit all the uses of the old SSA def, update to the
	// surviving ssa input, and update all the value numbers...)
	//
	BasicBlock* const ambBlock = jti.m_ambiguousVNBlock;
	if ((ambBlock != nullptr) && jti.m_block->KindIs(BBJ_COND) && (jti.m_block->GetUniquePred(this) == ambBlock))
	{
	JITDUMP(FMT_BB " has just one remaining predecessor " FMT_BB "\n", jti.m_block->bbNum, ambBlock->bbNum);
	Statement* const stmt = jti.m_block->lastStmt();
	assert(stmt != nullptr);
	GenTree* const jumpTree = stmt->GetRootNode();
	assert(jumpTree->OperIs(GT_JTRUE));
	GenTree* const tree = jumpTree->AsOp()->gtOp1;
	assert(tree->OperIsCompare());
	ValueNum treeOldVN = tree->GetVN(VNK_Liberal);
	ValueNum treeNormVN = ValueNumStore::NoVN;
	ValueNum treeExcVN = ValueNumStore::NoVN;
	vnStore->VNUnpackExc(treeOldVN, &treeNormVN, &treeExcVN);
	ValueNum treeNewVN = vnStore->VNWithExc(jti.m_ambiguousVN, treeExcVN);
	tree->SetVN(VNK_Liberal, treeNewVN);
	JITDUMP("Updating [%06u] liberal VN from " FMT_VN " to " FMT_VN "\n", dspTreeID(tree), treeOldVN, treeNewVN);
	}

I suppose if we are going to change things we should try to do so consistently. But it is not generally possible to incrementally repair VNs.

[EgorBo]

@AndyAyersMS I've filed a PR that handles it slightly simpler and in a more general way (and more diffs): #126917