JIT: Make effect handling in lowering less conservative

[jakobbotsch]

The interference checking in lowering bases some of its checks on GenTree::gtFlags. This is conservative since it includes effect flags of operands, which does not make much sense for LIR.

This PR replaces the relevant uses of gtFlags with a new GenTree::OperEffects() that computes the relevant effect flags for the node, excluding operands. We already know how to recompute effect flags other than GTF_GLOB_REF and GTF_ORDER_SIDEEFF. This PR adds functions for these as well (the GTF_GLOB_REF version
GenTree::OperRequiresGlobRefFlag is courtesy of @SingleAccretion).

For GTF_ORDER_SIDEEFF we add a GenTree::OperSupportsOrderingSideEffect which captures explicitly (and conservatively) the current cases where we are setting the flag, and only allows these cases to support the flag. Setting the flag for other cases may result in the flag being removed or ignored.

Fix #92699

Example:

public struct Point
{
    public long X, Y;

    public static unsafe bool Equals(in Point left, in Point right)
    {
        return (left.X == right.X) & (left.Y == right.Y);
    }
}

Codegen before:

G_M28859_IG02:  ;; offset=0x0008
            ldr     x2, [x0]
            ldr     x3, [x1]
            cmp     x2, x3
            cset    x2, eq
            ldr     x0, [x0, #0x08]
            ldr     x1, [x1, #0x08]
            cmp     x0, x1
            cset    x0, eq
            and     w0, w2, w0

Codegen after:

G_M28859_IG02:  ;; offset=0x0008
            ldr     x2, [x0]
            ldr     x3, [x1]
            ldr     x0, [x0, #0x08]
            ldr     x1, [x1, #0x08]
            cmp     x2, x3
            ccmp    x0, x1, 0, eq
            cset    x0, eq

[ghost]

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

Issue Details

---

The interference checking in lowering bases some of its checks on GenTree::gtFlags. This is conservative since it includes effect flags of operands. For LIR this does not really make sense and ends up being conservative.

This PR replaces the relevant uses of gtFlags with a new GenTree::OperEffects() that computes the relevant effect flags for the node, excluding operands. We already know how to recompute effect flags other than GTF_GLOB_REF and GTF_ORDER_SIDEEFF. This PR adds functions for these as well (the GTF_GLOB_REF version
GenTree::OperRequiresGlobRefFlag is courtesy of @SingleAccretion).

For GTF_ORDER_SIDEEFF we add a GenTree::OperSupportsOrderingSideEffect which captures explicitly (and conservatively) the current cases where we are setting the flag, and only allows these cases to support the flag. Setting the flag for other cases may result in the flag being removed or ignored.

Fix #92699

Author:	jakobbotsch
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @BruceForstall @kunalspathak

Diffs, mainly from new containment opportunities and ccmp opportunities like in the issue.
TP diffs are mostly positive, except for the native linux-x64 TP runs that seem to be somewhat worse affected, with a libraries.pmi outlier of up to +0.21%.

[jakobbotsch]

/azp run Fuzzlyn, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[SingleAccretion]

I also had an assert(!isInvariant || OperIs(GT_IND, GT_BLK)); here to exclude stores. Worth adding?

[jakobbotsch]

Added it

[SingleAccretion]

Suggested change

	//
	//
	// Globally visible stores and loads, as well as some equivalently modeled
	// operations, require the GLOB_REF flag to be set on the node.
	//

[jakobbotsch]

Expanded on this a bit in the remarks.

[SingleAccretion]

Do you have ideas on how to make sure that this method does not silently drop ordering effects when we start using it for new nodes?

For example, say someone is implementing non-null assertion propagation for GT_ARR_LENGTH family of operators. They will set the flag, just as current code does for indirs, and it will show up in dumps and all, but the effect will be silently dropped.

Perhaps add a setter for GTF_ORDER_SIDEEFF with an assert (there is SetAllEffectsFlags & Co already, but they are used to propagate flags upwards, so, not very suitable)?

[jakobbotsch]

I can add assert in fgdiagnostic to ensure that GTF_ORDER_SIDEEFF implies that OperSupportsOrderingSideEffect() || (any operand has the flag).

[jakobbotsch]

Doing this requires making sure we propagate/unset these flags in all the same places as the other flags... That's probably a good thing to do down the road, but I think for now I will skip it and think about doing that separately.
Let me add something like SetHasOrderingSideEffect() with the assert in it for this PR.

[jakobbotsch]

Doing this did make me notice that I missed GT_NULLCHECK in OperSupportsOrderingSideEffect though, so definitely worth to have something.

[SingleAccretion]

This was apparently in my second commit - GT_ALLOCOBJ should be here as well with return AsAllocObj()->gtHelperHasSideEffects;

[jakobbotsch]

The failures are known according to build analysis. The Fuzzlyn failures look like #92537