Optimization. Use Min/Max intrinsics if one of arguments is constant.

[SkiFoD]

fixes #65625
@tannergooding I had spent some time and implemented HW intrinstics for Min/Max and as asmdiffs shows more regressions I assume I did it wrong. I also couldn't figured it how to force the containment, I've always gotten something like this:

if I understand the role of "containment" right then vmovss must go away.

Alternatively, I tried to investigagate the lowering way and change ContainCheckIntrinsic method, but ended up with the IsContainableMemoryOp check is never true for Math.Min/Math.Max. The only option I had was marking both op's with SetRegOptional which doesn't seem right.

I would be so happy if you pushed me to the right direction :)

This PR was created due #65700 had been closed.

[ghost]

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

Issue Details

---

@tannergooding I had spent some time and implemented HW intrinstics for Min/Max and as asmdiffs shows more regressions I assume I did it wrong. I also couldn't figured it how to force the containment, I've always gotten something like this:

if I understand the role of "containment" right then vmovss must go away.

Alternatively, I tried to investigagate the lowering way and change ContainCheckIntrinsic method, but ended up with the IsContainableMemoryOp check is never true for Math.Min/Math.Max. The only option I had was marking both op's with SetRegOptional which doesn't seem right.

I would be so happy if you pushed me to the right direction :)

This PR was created due #65700 had been closed.

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

[tannergooding]

I had spent some time and implemented HW intrinstics for Min/Max and as asmdiffs shows more regressions I assume I did it wrong. I also couldn't figured it how to force the containment, I've always gotten something like this:

Could you share what C# code you're using to get this. At first glance this looks like it might be a loop and so the relevant data may be getting hoisted.

A simple: Max(x, y[i]) for float x and float[] y should be enough to trigger containment here.

[SkiFoD]

I had spent some time and implemented HW intrinstics for Min/Max and as asmdiffs shows more regressions I assume I did it wrong. I also couldn't figured it how to force the containment, I've always gotten something like this:

Could you share what C# code you're using to get this. At first glance this looks like it might be a loop and so the relevant data may be getting hoisted.

A simple: Max(x, y[i]) for float x and float[] y should be enough to trigger containment here.

Yes, you are right. I was using this loop to force it to be optimized:

 for(int i = 0; i <= 300000;i++)
  {
      result +=  Math.Min(x, y);
  }

There are not JitDisasm output when I use these flags: COMPlus_TieredCompilation=0 COMPlus_JITMinOpts=1, so I used the loop :)
Is there any way to fix this behaviour?

C# code as is return Math.Min(x, y[0]) produces this:

[jakobbotsch]

COMPlus_JITMinOpts=1

This will disable all JIT optimizations so you should not set this.
If you are not seeing JitDisasm it's typically because your function was inlined, make sure to mark it with [MethodImpl(MethodImplOptions.NoInlining)] to prevent that. If you post the full disassembly and C# code instead of just snippets it's easier for us to figure out what might be wrong.

[SkiFoD]

You are right, my code was inlined so there was not any output. Thank you, now It works 💯

@tannergooding @jakobbotsch What do you think of this implementation?
https://github.com/dotnet/runtime/blob/a9fbdbee8052bd14674d8712d1cd69b45dcdb0ef/src/coreclr/jit/importer.cpp#L4525-L4545
I chose SSE as minimum required for the optimization, is that right?

[tannergooding]

Given the assert and that we now have GT_CNS_VEC, this should probably be:

GenTreeVecCon* vecCon = gtNewVconNode(TYP_SIMD16, callJitType);

if (callJitType == CORINFO_TYPE_FLOAT)
{
    vecCon->gtSimd16Val.f32[0] = (float)op1->AsDblCon()->gtDconVal;
}
else
{
    vecCon->gtSimd16Val.f64[0] = op1->AsDblCon()->gtDconVal;
}

op1 = vecCon;

[tannergooding]

Otherwise the JIT is just going to have to convert the CreateScalarUnsafe back to a GT_CNS_VEC later in morph.

[SkiFoD]

@tannergooding Could you assist me with figuring out why I'm getting compilation failures during spmi replays. It feels like I'm missing something.

[tannergooding]

Its not clear to me at a glance. Many methods are failing with missing key "key" in map ....

The PR isn't changing the JIT/EE version and isn't adding anything that should really cause an issue here.

Maybe try merging in latest dotnet/main and seeing if it was just a CI issue

[jakobbotsch]

We don't do a good job of surfacing the assertion failures in superpmi-replay pipeline -- it takes some digging (usually jit devs run the replays locally, where it is more visible). In this case the failure is:

[18:57:25] ISSUE: <ASSERT> #45239 D:\a\_work\1\s\src\coreclr\jit\importer.cpp (4600) - Assertion failed 'ni == NI_System_Math_Min' in 'C:M(double):int' during 'Importation' (IL size 54; hash 0x97f702cc; FullOpts)

which looks related to the change.

[SkiFoD]

@tannergooding @jakobbotsch I made some adjustments, but CI failed with:

Assert failure(PID 5132 [0x0000140c], Thread: 4600 [0x11f8]): m_alignpad == 0 
File: D:\a\_work\1\s\src\coreclr\vm\syncblk.cpp Line: 2952

runtime/src/coreclr/vm/syncblk.cpp

Lines 2947 to 2958 in 6be8d27

	#if defined(HOST_64BIT) && defined(_DEBUG)
	void ObjHeader::IllegalAlignPad()
	{
	WRAPPER_NO_CONTRACT;
	#ifdef LOGGING
	void** object = ((void**) this) + 1;
	LogSpewAlways("\n\n******** Illegal ObjHeader m_alignpad not 0, object" FMT_ADDR "\n\n",
	DBG_ADDR(object));
	#endif
	_ASSERTE(m_alignpad == 0);
	}
	#endif // HOST_64BIT && _DEBUG

during System.Text.Json tests.

I run the tests on my local machine and couldn't reproduce the failure. I also run Fuzzlyn to find an issue, but got nothing.
Do you have any idea, how my code changes may cause the assert to fail?

[jakobbotsch]

That one looks like #70231

[SkiFoD]

@jakobbotsch Is there a way to restart CI pipelines? It seems like the current run is stuck.

[jakobbotsch]

@SkiFoD I've restarted the failing jobs.

[SkiFoD]

@tannergooding @jakobbotsch Yay, looks like it is working. Thank you a lot for your help. I have learnt a lot 👍

[tannergooding]

LGTM. Assuming @jakobbotsch has no more feedback I think this can be merged

[jakobbotsch]

LGTM to me too