Take this explicitly to avoid argument shuffling by caller

[xtqqczze]

No description provided.

[xtqqczze]

@MihuBot

[huoyaoyuan]

I don't think this is the right thing to do. It's just saving one mov in exceptional path, which is not performance critical, and likely won't have any impact on modern CPU because of register naming.

Moreover, emitting static local function as static is quite an implementation detail of C# compiler.

[xtqqczze]

@MihuBot arm64

[xtqqczze]

It's just saving one mov in exceptional path, which is not performance critical

True, but it does impact code size.

Moreover, emitting static local function as static is quite an implementation detail of C# compiler.

Presumably relying on an implementation detail is acceptable in System.Private.CoreLib.

[MihaZupan]

Wouldn't this also make things worse after inlining if this can be entirely omitted currently?

[huoyaoyuan]

It's just saving one mov in exceptional path, which is not performance critical

True, but it does impact code size.

There are only several bytes of difference with no real impact. In practice, we would take the opposite approach that accepts trivial regression to simplify the code.

[xtqqczze]

This pattern appears elsewhere:

runtime/src/libraries/System.Text.Encodings.Web/src/System/Text/Encodings/Web/DefaultHtmlEncoder.cs

Lines 98 to 100 in 0f6c3d8

	#pragma warning disable IDE0060 // 'this' taken explicitly to avoid argument shuffling by caller
	static int TryEncodeScalarAsHex(object @this, uint scalarValue, Span<byte> destination)
	#pragma warning restore IDE0060

[xtqqczze]

@MihuBot -arm

[huoyaoyuan]

This pattern appears elsewhere:

The trick in referenced code is outdated: sharplab

It saves literally one mov only, and the resultant asm size has actually regressed because of GPR encoding of extra register (r10 vs rdx. We should get rid of the trick.

[xtqqczze]

We should get rid of the trick.

xtqqczze@91a5fc2: MihuBot/runtime-utils#861

[xtqqczze]

MihuBot/runtime-utils#860

@MihaZupan Interesting there is an improvement of 764 bytes on arm64, only 16 bytes on X64

[xtqqczze]

We should get rid of the trick.

xtqqczze@91a5fc2: MihuBot/runtime-utils#861

@huoyaoyuan I see regressions in code size and PerfScore with the trick removed

[huoyaoyuan]

@huoyaoyuan I see regressions in code size and PerfScore with the trick removed

The "regression" is very unlikely to be impactful. PerfScore is an internal JIT term for optimization decisions. We shouldn't tweak code too much to fit the JIT, but teach JIT instead.
I don't expect there will be any impact on performance benchmark numbers - that's the most important thing.

[jkotas]

This pattern appears elsewhere:

runtime/src/libraries/System.Text.Encodings.Web/src/System/Text/Encodings/Web/DefaultHtmlEncoder.cs

This is not an exception throwing path. The trick is potentially saving extra instructions on a hot path here. It is still debatable whether this micro-optimization is worth the extra complexity.

[jkotas]

Interesting there is an improvement of 764 bytes on arm64

This improvement does not look real. It is an issue in asm-diff tooling.

[jkotas]

Thank you for a contribution, but we are not interested in complicating the code like this to save a few instructions on exception throwing paths.