[JIT] Rationalize: Re-run pre-order processing on identity shuffle rewrite in PreOrderVisit

[adamperlin]

Resolves #133787; This case comes up with IR that looks like the following:

STOREIND simd16
├── LCL_VAR V00 RetBuf
└── HWINTRINSIC Shuffle                         [000022]
    ├── HWINTRINSIC ShiftRightLogical128BitLane [000020]
    │   ├── IND simd16
    │   │   └── LCL_VAR V01 arg0
    │   └── CAST int <- ubyte
    │       └── LCL_VAR V02 arg1
    └── CNS_VEC <0, 1, 2, ..., 15>              [000016]

The outer shuffle is the identity, so it is folded away as part of RewriteHwIntrinsicAsUserCall during PreOrderVisit. The inner ShiftRightLogical128BitLane -- which needs to be re-written back to a GT_CALL since it has a non-constant arg -- is the replacement node, but it is never processed by the preorder visitor. If we get back a different node from Rewrite*Intrinsic, we should re-do the processing logic on that new node.

[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.

[dotnet-policy-service]

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

[adamperlin]

@dotnet/jit-contrib PTAL
cc @EgorBo

[Copilot]

Copilot review overview

🟡 Changes recommended

The reported Checked-build regression needs a targeted test to prevent recurrence.

Review effort: Balanced
Findings: 1

Open (1)

• Add JIT regression test for non-constant Vector128.Shuffle immediate · New

What changed in this PR

Fixes JIT rationalization so replacement intrinsic nodes receive pre-order processing.

Changes:

• Repeats intrinsic rewriting until the node remains unchanged.
• Ensures nested deferred hardware intrinsics are converted to user calls.

File	Description
src/​coreclr/​jit/​rationalize.cpp	Reprocesses rewritten intrinsic nodes during rationalization.

[tannergooding]

Is this a potential algorithmic complexity problem? Can we reduce the amount of looping/walking we need to do somehow as presumably this is uncommon and doesn't need everything to rewalk?

[jakobbotsch]

I agree, can the transformation which does this instead ensure that it does the necessary processing before returning?

[adamperlin]

I believe there is a potential algorithmic complexity problem for a parent-child chain of identity-shuffle's that are eliminated:

SHUFFLE_1 identity [deferred user call] 
 |____ SHUFFLE_2 identity 
 ...
       |___SHUFFLE_N identity 
            |__ T

Each iteration of RewriteHWIntrinsicAsUserCall strips one level away and re-runs fgSetTreeSeq over the child (unchanged) sub-tree chain at each iteration, leading to O(n^2) behavior where n is the chain length. Moving the re-processing down into RewriteHWIntrinsicAsUserCall doesn't necessarily fix the complexity issue in this worst case, but we could maybe optimize specifically the identity shuffle case to avoid re-processing unchanged sub-trees?

[adamperlin]

@tannergooding I believe the issue should be addressed now; I added an optimized path for when we have an identity shuffle that returns one of its operands unchanged.

[jakobbotsch]

Can the path that creates the new operand or moves it into a position where it won't be visited naturally just call the walk recursively on it?

Modifying the root visit seems like a big hammer and it also has non trivial TP costs.

[adamperlin]

Got it, that makes sense. I updated this PR to pass the visitor through to RewriteHWIntrinsicAsUserCall so that we can re-invoke the PreOrderVisit on the path that moves the operand!

[adamperlin]

@tannergooding can you take another look here?