JIT: Remove "add copies" phase

[jakobbotsch]

This phase seems to try to do some ad-hoc live range splitting to improve things in assertion prop, but it almost barely kicks in due to the following checks:

runtime/src/coreclr/jit/assertionprop.cpp

Lines 554 to 580 in 588dcd7

	// If locals must be initialized to zero, that initialization counts as a second definition.
	// VB in particular allows usage of variables not explicitly initialized.
	// Note that this effectively disables this optimization for all local variables
	// as C# sets InitLocals all the time starting in Whidbey.
	if (!varDsc->lvIsParam && info.compInitMem)
	{
	continue;
	}
	// On x86 we may want to add a copy for an incoming double parameter
	// because we can ensure that the copy we make is double aligned
	// where as we can never ensure the alignment of an incoming double parameter
	//
	// On all other platforms we will never need to make a copy
	// for an incoming double parameter
	bool isFloatParam = false;
	#ifdef TARGET_X86
	isFloatParam = varDsc->lvIsParam && varTypeIsFloating(typ);
	#endif
	if (!isFloatParam && !varDsc->lvVolatileHint)
	{
	continue;
	}

When it does kick in it seems to overall be a regression (diffs), both in ASM diffs and in TP diffs.
Furthermore, we pay 16 bytes in every LclVarDsc (out of 88) for bookkeeping purposes for this pass, even in MinOpts.

[ghost]

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

Issue Details

---

null

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

[EgorBo]

Remove PHASE_OPTIMIZE_ADD_COPIES from compphases.h ?

[jakobbotsch]

Whoops, good catch, done!

[EgorBo]

Nice wins actually 😄 (except TP on arm32)

[jakobbotsch]

Nice wins actually 😄 (except TP on arm32)

This is likely just an artifact of the decreased size of LclVarDsc, I will push this without the fields removed to see

[jakobbotsch]

Diffs when the fields are left in, but with the pass itself removed.

cc @dotnet/jit-contrib -- I propose that we remove this phase. The phase itself is overall a regression to have around, both on TP and ASM diffs. In addition it requires 16 bytes in LclVarDsc (out of 88 on x64) that we pay for even in MinOpts.
The phase seems to try to do some ad-hoc live-range splitting before assertion prop, but it almost never kicks in because of these checks:

runtime/src/coreclr/jit/assertionprop.cpp

Lines 554 to 580 in 588dcd7

	// If locals must be initialized to zero, that initialization counts as a second definition.
	// VB in particular allows usage of variables not explicitly initialized.
	// Note that this effectively disables this optimization for all local variables
	// as C# sets InitLocals all the time starting in Whidbey.
	if (!varDsc->lvIsParam && info.compInitMem)
	{
	continue;
	}
	// On x86 we may want to add a copy for an incoming double parameter
	// because we can ensure that the copy we make is double aligned
	// where as we can never ensure the alignment of an incoming double parameter
	//
	// On all other platforms we will never need to make a copy
	// for an incoming double parameter
	bool isFloatParam = false;
	#ifdef TARGET_X86
	isFloatParam = varDsc->lvIsParam && varTypeIsFloating(typ);
	#endif
	if (!isFloatParam && !varDsc->lvVolatileHint)
	{
	continue;
	}

The first check there also means it kicks in even less in customer code that doesn't have SkipLocalsInit.