Implement the reuse of duplicate constant values #43899

briansull · 2020-10-27T17:54:24Z

Duplicate values will now be reused in the constants area
Added MIN_DATA_ALIGN and MAX_DATA_ALIGN constants
Added dsDataType to record the type of the constant
Print the floating point values in the constants area
Changed 'alignment' and 'cnsSize' parameters to be unsigned instead of UNATIVE_OFFSET
Added method emitDataGenFind to search and return a duplicate constant value

- Duplicate values will now be reused in the constants area - Added MIN_DATA_ALIGN and MAX_DATA_ALIGN constants - Added dsDataType to record the type of the constant - Print the floating point values in the constants area - Changed 'alignment' and 'cnsSize' parameters to be unsigned instead of UNATIVE_OFFSET - Added method emitDataGenFind to search and return a duplicate constant value

briansull · 2020-10-27T17:54:36Z

Fixes #42178

briansull · 2020-10-27T17:59:47Z

Sample diffs:

; Assembly listing for method Number:TryParseDouble(ReadOnlySpan`1,int,NumberFormatInfo,byref):bool
; Emitting BLENDED_CODE for X64 CPU with SSE2 - Windows
; ReadyToRun compilation
; optimized code
; rbp based frame
; partially interruptible

G_M10185_IG33:
<       movsd    xmm0, qword ptr [reloc @RWD24]
>       movsd    xmm0, qword ptr [reloc @RWD00]

G_M10185_IG39:
<       movsd    xmm0, qword ptr [reloc @RWD32]
>       movsd    xmm0, qword ptr [reloc @RWD16]

G_M10185_IG51:
<       movsd    xmm0, qword ptr [reloc @RWD40]
>       movsd    xmm0, qword ptr [reloc @RWD16]

<RWD00  dq	7FF0000000000000h
<RWD08  dq	FFF0000000000000h
<RWD16  dq	FFF8000000000000h
<RWD24  dq	7FF0000000000000h
<RWD32  dq	FFF8000000000000h
<RWD40  dq	FFF8000000000000h

>RWD00  	dq	7FF0000000000000h		;          inf
>RWD08  	dq	FFF0000000000000h		;         -inf
>RWD16  	dq	FFF8000000000000h		;    -nan(ind)

briansull · 2020-10-28T16:43:32Z

@BruceForstall @CarolEidt PTAL

src/coreclr/src/jit/emit.cpp

BruceForstall · 2020-10-28T18:52:59Z

src/coreclr/src/jit/emit.cpp

-    UNATIVE_OFFSET cnum = emitDataGenBeg(cnsSize, cnsAlign);
-    emitDataGenData(0, cnsAddr, cnsSize);
-    emitDataGenEnd();
+    UNATIVE_OFFSET cnum = emitDataGenFind(cnsAddr, cnsSize, cnsAlign, dataType);


You've introduced an O(n) search for each constant. Presumably, you could construct a function with a large number of constants where this might show up on a trace. It seems like the usual case would be very few searches, but the usual case is also probably very few matches (duplicates). Would be interesting to see some stats about this across our SPMI collections.

Yes, I understand that. There are very few constants used by the JIT, so I didn't think it was worth adding a hash table for this. I can add a check that counts the total number and asserts (in a checked JIT) when it exceeds a certain limit, (like 10,000 or so)

I'm not sure it's worth an assert. I suppose the search function could bail out after, say, 100 constants and just always add a new (possibly duplicated) constant after that.

That is another possibility. Let me run the tests and SPMI to see if we even have this case today.
Using 10k we would hits this after have 142 unique constants and/or switch tables in a single method.

I settled on only checking for matches in the first 64 entries and the adding the new value if is doesn't match

// If we don't find a match in the first 64, then we just add the new constant // This prevents an O(n^2) search cost

CarolEidt

Overall LGTM - I'm not sure exactly how the dump looks now, but it seems to be a good improvement - thanks!
I'd agree with Bruce's suggestion to consider a limit on the number of reuse constants.

src/coreclr/src/jit/emit.cpp

CarolEidt

LGTM - thanks!

src/coreclr/src/jit/emit.cpp

tannergooding · 2020-10-29T14:01:22Z

src/coreclr/src/jit/emit.cpp

-    // This restricts the alignment to: 1, 2, 4, 8, 16, or 32 bytes
-    // Alignments greater than 32 would require VM support in ICorJitInfo::allocMem
+    // This restricts the alignment to MAX_DATA_ALIGN
+    // Alignments greater than 32 also require VM support in ICorJitInfo::allocMem


Suggested change

// Alignments greater than 32 also require VM support in ICorJitInfo::allocMem

// Alignments greater than MAX_DATA_ALIGN also require VM support in ICorJitInfo::allocMem

Actually you have add support for alignments greater than 32 to ICorJitInfo::allocMem, before you can increase the emitter's MAX_DATA_ALIGN. If we ever do that we may also move the definitions of MAX/MIN_DATA_ALIGN out of emit.h and into compiler.h

Should the comment possibly be moved to be on MAX_DATA_ALIGN and indicate something like "this can't be increased without also touching ICorJitInfo::allocMem"

tannergooding · 2020-10-29T14:07:54Z

src/coreclr/src/jit/emit.cpp

+    {
+        // Search the existing secDesc entries
+
+        if ((secDesc->dsSize == cnsSize) && (secDesc->dsDataType == dataType) && ((curOffs % alignment) == 0))


Can we not just match the raw integral bits rather than needing to check "dataType"?

For example, if you had a ulong constant that was 0x8000_0000_0000_0000 you could also use it for a double constant that was -0.0 as they have the same 64-bit pattern. This would likely be useful for scenarios where you are doing floating-point algorithms that are manipulating the underlying bits.

Likewise, I imagine this might be less efficient for certain SIMD code. If you emit a SIMD constant for <0.0f, 1.0f, 2.0f, 3.0f> and also have a floating-point constant for 1.0f then we will have both emitted separately, rather than just having the latter load from element 1 of the SIMD constant.

These might just be future optimization opportunities, but I wanted to call them out.

I could make that change. It would probably match floating point zero to integer zero frequently.
I also could do (secDesc->dsSize >= cnsSize) as well.

It would probably match floating point zero to integer zero frequently

I think this is probably a rare case. On ARM there is the explicit zero register and on x86 I think we prefer xor reg, reg and xorps reg, reg as they are both smaller code and elided by the instruction decoder.

tannergooding · 2020-10-29T14:17:30Z

src/coreclr/src/jit/emit.cpp

+                switch (dsc->dsDataType)
+                {
+                    case TYP_FLOAT:
+                        printf(" ; float  %9.6g", (double)*reinterpret_cast<float*>(&dsc->dsCont));


Why is this converting to double for printf rather than just keeping it as float?

AFAIK printf() only supports the printing of doubles

The C calling convention only passes doubles, not floats

👍, I'm definitely not as up to date with what the official standards support.

A simple MSVC console app, however, looks to work and Clang/GCC don't give any warnings on compile.

tannergooding · 2020-10-29T14:31:39Z

src/coreclr/src/jit/emit.cpp

+                        printf(" ; float  %9.6g", (double)*reinterpret_cast<float*>(&dsc->dsCont));
+                        break;
+                    case TYP_DOUBLE:
+                        printf(" ; double %12.9g", *reinterpret_cast<double*>(&dsc->dsCont));


Perhaps it would be better to use .17g which guarantees it is uniquely representable (but may be overly verbose) or also print the underlying 64-bits as hex. As it is now, this will cause values to look identical in the log when they may not be in practice.

In C#, I typically do ${x} (0x{BitConverter.DoubleToInt64Bits(x):X16}", but we also ensures double.ToString and float.ToString are the "shortest roundtrippable string" by default now.

C++ 17 has std::to_chars which also ensures the result is the "shortest roundtrippable string" (provided it is reparsed by std::from_chars).

We already have printed the binary value.
Additionally we print the floating point value as a comment in the Asm output to help the user see what value is being loaded.

👍, I saw us printing the binary value elsewhere but wasn't sure where in the dump that was relative to this.

It would still be nice if we could ensure that values don't conflict in other places though (IMO).
For example, trimming double to 9 digits will result in both of the following looking equal:

3.141592653589793 (0x400921FB54442D18) -- Math.PI 3.14159265 (0x400921FB53C8D4F1)

However, they have 8 million uniquely representable values that exist between them.

src/coreclr/src/jit/emit.cpp

Allow binary bit pattern matches of constants

briansull · 2020-10-29T17:47:46Z

Updated with Tanner's suggestions

tannergooding

LGTM!

Thanks for the improvements!

briansull · 2020-10-29T20:25:13Z

@BruceForstall You are still listed as requested changes (which I have made)

Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 27, 2020

briansull linked an issue Oct 27, 2020 that may be closed by this pull request

RyuJIT: inst_RV_RV_TT doesn't check if constants already exists in the data section #42178

Closed

briansull assigned BruceForstall Oct 27, 2020

briansull requested review from CarolEidt, brjohnstmsft and BruceForstall October 27, 2020 17:55

briansull assigned briansull and unassigned BruceForstall Oct 27, 2020

briansull removed the request for review from brjohnstmsft October 27, 2020 17:56

Correctly place declaration of curOffs

1041e86

briansull mentioned this pull request Oct 28, 2020

[WIP] Better data sections, pool constants, print floating point values, improve diffable, CSE float constants #43453

Closed

BruceForstall suggested changes Oct 28, 2020

View reviewed changes

CarolEidt reviewed Oct 28, 2020

View reviewed changes

src/coreclr/src/jit/emit.cpp Outdated Show resolved Hide resolved

briansull added 2 commits October 29, 2020 00:12

Code review feedback

186bc4b

Carol's feedback

cebf0ae

CarolEidt approved these changes Oct 29, 2020

View reviewed changes

tannergooding reviewed Oct 29, 2020

View reviewed changes

src/coreclr/src/jit/emit.cpp Outdated Show resolved Hide resolved

tannergooding reviewed Oct 29, 2020

View reviewed changes

src/coreclr/src/jit/emit.cpp Outdated Show resolved Hide resolved

Feedback from Tanner Gooding

56bf515

Allow binary bit pattern matches of constants

briansull force-pushed the pooled-constants branch from 80f63b4 to 56bf515 Compare October 29, 2020 17:46

tannergooding approved these changes Oct 29, 2020

View reviewed changes

briansull added 2 commits October 29, 2020 11:52

Fix build warning/error

46e0e98

Jit Format

199d01a

BruceForstall approved these changes Oct 29, 2020

View reviewed changes

briansull merged commit 4d09ba6 into dotnet:master Oct 30, 2020

ghost locked as resolved and limited conversation to collaborators Dec 6, 2020

	// Alignments greater than 32 also require VM support in ICorJitInfo::allocMem
	// Alignments greater than MAX_DATA_ALIGN also require VM support in ICorJitInfo::allocMem

Implement the reuse of duplicate constant values #43899

Implement the reuse of duplicate constant values #43899

Uh oh!

Conversation

briansull commented Oct 27, 2020

Uh oh!

briansull commented Oct 27, 2020

Uh oh!

briansull commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

briansull commented Oct 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

briansull commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding left a comment

Choose a reason for hiding this comment

Uh oh!

briansull commented Oct 29, 2020

Uh oh!

Uh oh!

briansull commented Oct 27, 2020 •

edited

Loading

tannergooding Oct 29, 2020 •

edited

Loading

briansull commented Oct 29, 2020 •

edited

Loading