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

I remember there was some number of std::swaps in gentree.cpp that were responsible for the reordering, e. g.:

They need to be dealt with in some way, as it is very surprising (-- prone to creating subtle bugs) for a gtNewXYZ function to potentially silently reorder operands (at a minimum, it needs to assert that it can do that safely, with gtCanSwapOrder(op1, op2)).

Additionally, it would be nice to add tests for all affected intrinsics (not just AndNot).

@fanyang-mono, could you point me to where Mono makes decisions about side effects?

It looks like it has the same issue where its incorrectly re-ordering parameters for some intrinsics.

@fanyang-mono, could you point me to where Mono makes decisions about side effects?

It looks like it has the same issue where its incorrectly re-ordering parameters for some intrinsics.

I was able to reproduce the issue locally with fullaot. I need some time to investigate and will get back to you tmr.

This will fix the mono failure:
#70308

#70308 has merged, we should try re-running failing mono lanes

/azp run runtime

They need to be dealt with in some way, as it is very surprising (-- prone to creating subtle bugs) for a gtNewXYZ function to potentially silently reorder operands (at a minimum, it needs to assert that it can do that safely, with gtCanSwapOrder(op1, op2)).

I agree, I'll log a new issue to track them since they are impacted a different way and will require different handling.

Additionally, it would be nice to add tests for all affected intrinsics (not just AndNot).

I'll work on adding these.

CC. @dotnet/jit-contrib

Ping @dotnet/jit-contrib

Would be good to get this in for 7 to help ensure code works as users expect without accidental side effect re-ordering.

@AndyAyersMS do you have an example? Shouldn't impSpillSideEffect detect when a nested node also has side effects considering we rely on it for other phases of import?

We don't generally rely on it elsewhere; it is mainly called from impSpillSideEffects which is typically called with either NONE or ALL.

Test case would be something along these lines (might need to fiddle a bit to get it right). Call with d == 0.

    [MethodImpl(MethodImplOptions.NoInlining)]
    static Vector128<int> SideEffectsTree_Sse2_ShiftLeftLogical(Vector128<int>[] left, byte* right, int d)
    {
        if (Sse2.IsSupported)
        {
            return Sse2.ShiftLeftLogical(Sse2.ShiftLeftLogical(left[0], *right), 5 / d));

@AndyAyersMS, not sure I understand.

impSpillSideEffects effectively just does impSpillSideEffect for each stack argument from 0 (bottom) to chkLevel (which is frequently CHECK_SPILL_ALL).

So for CHECK_SPILL_ALL with 2 operands, it will spill op1, then op2; and only spill if that stack entry is marked as GTF_GLOB_EFFECT or GTF_SIDE_EFFECT -or- we're spilling global effects and its an address exposed local.

For most of these intrinsics we have a binary case where we need to swap operand 1 and 2. So we only check if op1 needs to be spilled. We shouldn't also need to check if op2 needs to be spilled because it will be evaluated after op1 if op1 is spilled first.

impSpillSideEffects itself wouldn't catch anything else since it doesn't do any additional checks on top outside impSpillSpecialSideEff() which we should never have since that only applies to exception objects. It would only potentially spill op2 when its unnecessary since it will always be the "next" operand in the evaluation order.

Hmmm. I guess, if the stack had 3 operands and the unrelated operand had side effects that could be problematic. In which case we'd want impSpillSideEffects(true, verCurrentState.esStackDepth - 2, "Reason")....

Is that a concern here? The stack should normally be balanced, right?

Looking at it some more I think what you have will be ok, if there are more levels to the tree, you'll just accumulate more op1 spills along the way.

Eg if abstractly there is some tree like (% (% a b) c) where the expansion of % wants to reverse operand order (call this %r), you'll emit

t0  = a;
t1  = (%r b, t0);
... = (%r c, t1);