JIT: Remove questionable transformation

[EgorBo]

No description provided.

[ghost]

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

Issue Details

---

null

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

[EgorBo]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

What IR does this create for the example case we were looking at?

[EgorBo]

It used to be:

    [000002] ----G+-----                         *  CAST      long <- int
    [000001] n---G+-----                         \--*  IND       byte  
    [000000] H----+-----                            \--*  CNS_INT(h) long   0x7ffa2213f21a static Fseq[s_13]

to

    [000002] ----G+-----                         *  NOP       byte  
    [000001] n---G+-----                         \--*  IND       byte  
    [000000] H----+-----                            \--*  CNS_INT(h) long   0x7ffa2213f21a static Fseq[s_13]

Now it's:

    [000002] ----G+-----                         *  CAST      long <- int
    [000001] n---G+-----                         \--*  IND       byte  
    [000000] H----+-----                            \--*  CNS_INT(h) long   0x7ffa22c7f21a static Fseq[s_13]

to

    [000002] ----G+-----                         *  CAST      int <- byte <- int
    [000001] n---G+-----                         \--*  IND       byte  
    [000000] H----+-----                            \--*  CNS_INT(h) long   0x7ffa22c7f21a static Fseq[s_13]

[jakobbotsch]

That's still an illegal transformation in this case since the value was an argument. The new cast does not guarantee widening into the upper 32 bits like the original cast does.

[EgorBo]

@jakobbotsch are you suggesting to remove the whole transformation?

[jakobbotsch]

It seems there are two separate questions:

1. Who is invoking optNarrowTree in this way that changes semantics of the program? The transformation is questionable to me but it's because the end result is a change change in behavior -- and this PR still has that problem.
2. Can we get rid of the weird passthrough GT_NOP? Yes, and this PR probably accomplishes that, but it seems a better way would be to refactor optNarrowTree to take a GenTree** so that it can just replace the use of the cast with the operand in this situation.

I'm more worried about (1) than (2), though it would also be nice to clean up (2).

[SingleAccretion]

If you are deleting this quirky NOP variant, you can also delete code here:

runtime/src/coreclr/jit/valuenum.cpp

Lines 11371 to 11375 in 721c445

	if (tree->OperIs(GT_NOP))
	{
	// Pass through arg vn.
	tree->gtVNPair = tree->AsOp()->gtOp1->gtVNPair;
	}

and case GT_NOP (as well as adjust the comment) here:

runtime/src/coreclr/jit/rationalize.cpp

Lines 252 to 264 in 721c445

	case GT_NOP:
	case GT_BOX:
	case GT_ARR_ADDR:
	// "optNarrowTree" sometimes inserts NOP nodes between defs and uses.
	// In this case, remove the NOP. BOX/ARR_ADDR are such "passthrough"
	// nodes by design, and at this point we no longer need them.
	if (node->gtGetOp1() != nullptr)
	{
	use.ReplaceWith(node->gtGetOp1());
	BlockRange().Remove(node);
	node = node->gtGetOp1();
	}
	break;

[EgorBo]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

There's probably some more stuff to fix around optNarrowTree, but this looks good to me to unblock the Fuzzlyn runs.

gtEffectiveVal can be cleaned up too now if passthrough NOPs no longer exist... but doesn't necessarily have to be in this PR.