JIT: Merge more stores/loads by EgorBo · Pull Request #95823 · dotnet/runtime

EgorBo · 2023-12-09T14:31:56Z

class MyClass
{
    public byte A;
    public byte B;
    public char C;
    public int D;
}

void Test(MyClass c1, MyClass c2)
{
    c1.A = c2.A;
    c1.B = c2.B;
    c1.C = c2.C;
    c1.D = c2.D;
}

Codegen diff for Test:

; Method Program:Test(MyClass,MyClass):this (FullOpts)
-      movzx    rax, byte  ptr [r8+0x0E]
-      mov      byte  ptr [rdx+0x0E], al
-      movzx    rax, byte  ptr [r8+0x0F]
-      mov      byte  ptr [rdx+0x0F], al
-      movzx    rax, word  ptr [r8+0x0C]
-      mov      word  ptr [rdx+0x0C], ax
-      mov      eax, dword ptr [r8+0x08]
-      mov      dword ptr [rdx+0x08], eax
+      mov      rax, qword ptr [r8+0x08]
+      mov      qword ptr [rdx+0x08], rax
       ret      
-; Total bytes of code: 33
+; Total bytes of code: 9

For more diffs it needs a sort of "forward sub" in lowering to handle cases like

a[0] = b[0];
a[1] = b[1]

ghost · 2023-12-09T14:32:09Z

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

Issue Details

class MyClass
{
    public byte A;
    public byte B;
    public char C;
    public int D;
}

void Test(MyClass c1, MyClass c2)
{
    c1.A = c2.A;
    c1.B = c2.B;
    c1.C = c2.C;
    c1.D = c2.D;
}

Codegen diff for Test:

; Method Program:Test(MyClass,MyClass):this (FullOpts)
-      movzx    rax, byte  ptr [r8+0x0E]
-      mov      byte  ptr [rdx+0x0E], al
-      movzx    rax, byte  ptr [r8+0x0F]
-      mov      byte  ptr [rdx+0x0F], al
-      movzx    rax, word  ptr [r8+0x0C]
-      mov      word  ptr [rdx+0x0C], ax
-      mov      eax, dword ptr [r8+0x08]
-      mov      dword ptr [rdx+0x08], eax
+      mov      rax, qword ptr [r8+0x08]
+      mov      qword ptr [rdx+0x08], rax
       ret      
-; Total bytes of code: 33
+; Total bytes of code: 9

For more diffs it needs a sort of "forward sub" in lowering to handle cases like

a[0] = b[0];
a[1] = b[1]

etc

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

src/coreclr/jit/lower.cpp

am11 · 2023-12-10T07:36:46Z

src/coreclr/jit/lower.cpp

+    //
+    // IND<byte> is always fine (and all IND<X> created here from such)
+    // IND<simd> is not required to be atomic per our Memory Model
+    const bool allowsNonAtomic = data1.allowsNonAtomic && data2.allowsNonAtomic;


Would it make a difference (cover more cases) if it was a positive check: allowAtomic?

I am not sure I follow - can you clarify?

Was just wondering if the condition below actually care for "if atomics are allowed, do the transformation" instead of "if non-atomics are allowed, skip the transformation", then maybe it can be based on allowAtomic? Not sure if it's feasible or would make any difference. 😅

The idea that we assume that we can't use wider loads by default, we need to find a proof that we can and we only have a few hints for that today so it's quite conservative

EgorBo · 2023-12-11T16:08:09Z

@jakobbotsch PTAL since you reviewed the initial version cc @dotnet/jit-contrib

Diffs are not too big due to conservative alias analysis, but I have some future improvements in mind which might increase the coverage (e.g. GT_STORE_LCL_FLD)

jakobbotsch · 2023-12-11T16:15:49Z

src/coreclr/jit/lower.cpp

+//    *  STOREIND  int
+//    +--*  LEA(b+8)  byref
+//    |  \--*  LCL_VAR   ref
+//    \--*  IND       int
+//    \--*  LEA(b+8)  byref
+//        \--*  LCL_VAR   ref
+//
+//    *  STOREIND  int
+//    +--*  LEA(b+12) byref
+//    |  \--*  LCL_VAR   ref
+//    \--*  IND       int
+//    \--*  LEA(b+12) byref
+//        \--*  LCL_VAR   ref
+//
+//    is transformed into:
+//
+//    *  STOREIND  long
+//    +--*  LEA(b+8)  byref
+//    |  \--*  LCL_VAR   ref
+//    \--*  IND       long
+//    \--*  LEA(b+8)  byref
+//        \--*  LCL_VAR   ref


Indentation of these trees seems wrong.

jakobbotsch · 2023-12-11T16:23:53Z

src/coreclr/jit/lower.cpp

+    // Data is either a constant or GT_IND
+    // TODO-CoalescingStores: allow locals (to then broadcast them)
+    if (isStore && !ind->Data()->IsCnsIntOrI() && !ind->Data()->IsVectorConst() && !ind->Data()->OperIs(GT_IND))
    {
        return false;
    }


~~This logic seems like it needs some form of interference checking somewhere when the data is GT_IND. What if the definition occurs 200 nodes before, with interfering stores?~~ (nevermind, looks like LowerStoreIndirCoalescing takes care of it by virtue of ensuring things are contiguous)

jakobbotsch · 2023-12-11T16:27:44Z

src/coreclr/jit/lower.cpp

        // Get coalescing data for the previous STOREIND
        GenTreeStoreInd* prevInd = prevTree->AsStoreInd();
-        if (!GetStoreCoalescingData(comp, prevInd->AsStoreInd(), &prevData))
+        if (!GetLoadStoreCoalescingData(comp, prevInd->AsStoreInd(), &prevData) || !CanBeCoalesced(prevData, currData))


Ah, I guess the loop above this indirectly ensures that there won't be any interference between the two INDs.

jakobbotsch · 2023-12-11T16:31:37Z

src/coreclr/jit/lower.cpp

+            const int storeStart = min(currData.offset, prevData.offset);
+            const int loadStart  = min(currValueData.offset, prevValueData.offset);
+
+            const int smallerOffset = min(storeStart, loadStart);
+            const int largerOffset  = max(storeStart, loadStart);
+
+            if ((smallerOffset != largerOffset) && ((smallerOffset + (int)genTypeSize(newType)) > largerOffset))
+            {
+                // May alias
+                return;
+            }


Not sure I totally understand this logic. Can it use a normal interval intersection check? Like e.g.

runtime/src/coreclr/jit/promotion.cpp

Lines 1510 to 1533 in c9088fe

//------------------------------------------------------------------------

// Intersects:

// Check if this segment intersects another segment.

//

// Parameters:

// other - The other segment.

//

// Returns:

// True if so.

//

bool StructSegments::Segment::Intersects(const Segment& other) const

{

if (End <= other.Start)

{

return false;

}

if (other.End <= Start)

{

return false;

}

return true;

}

jakobbotsch · 2023-12-11T16:37:10Z

e.g. GT_STORE_LCL_FLD

When I checked LCL_FLD in relation to #92768 I found almost no cases where they could be optimized to ldp or stp, so it seems they are rare (or my check was wrong).

EgorBo · 2024-04-30T15:30:48Z

I'll re-think this one later

EgorBo added 2 commits December 9, 2023 15:04

Merge more stores

b1eab0b

Fix bug

47bf92e

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 9, 2023

ghost assigned EgorBo Dec 9, 2023

EgorBo added 3 commits December 9, 2023 17:06

Oops, fix copy-paste

469caa6

fix jit diffs

0f1f818

Alias analysis

a6bab5d

EgorBo marked this pull request as ready for review December 9, 2023 20:42

am11 reviewed Dec 10, 2023

View reviewed changes

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

am11 reviewed Dec 10, 2023

View reviewed changes

Address feedback

659f63c

build-analysis bot mentioned this pull request Dec 10, 2023

Test failure - System.NullReferenceException in System.Threading.Lock.TryInitializeStatics #94728

Closed

EgorBo requested a review from jakobbotsch December 11, 2023 16:08

jakobbotsch reviewed Dec 11, 2023

View reviewed changes

EgorBo closed this Apr 30, 2024

github-actions bot locked and limited conversation to collaborators May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Merge more stores/loads#95823

JIT: Merge more stores/loads#95823
EgorBo wants to merge 6 commits intodotnet:mainfrom
EgorBo:merge-more-stores

EgorBo commented Dec 9, 2023 •

edited

Loading

Uh oh!

ghost commented Dec 9, 2023

Uh oh!

Uh oh!

am11 Dec 10, 2023 •

edited

Loading

Uh oh!

EgorBo Dec 10, 2023

Uh oh!

am11 Dec 10, 2023

Uh oh!

EgorBo Dec 10, 2023

Uh oh!

EgorBo commented Dec 11, 2023 •

edited

Loading

Uh oh!

jakobbotsch Dec 11, 2023

Uh oh!

jakobbotsch Dec 11, 2023 •

edited

Loading

Uh oh!

jakobbotsch Dec 11, 2023

Uh oh!

jakobbotsch Dec 11, 2023

Uh oh!

jakobbotsch commented Dec 11, 2023

Uh oh!

EgorBo commented Apr 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	//------------------------------------------------------------------------
	// Intersects:
	// Check if this segment intersects another segment.
	//
	// Parameters:
	// other - The other segment.
	//
	// Returns:
	// True if so.
	//
	bool StructSegments::Segment::Intersects(const Segment& other) const
	{
	if (End <= other.Start)
	{
	return false;
	}

	if (other.End <= Start)
	{
	return false;
	}

	return true;
	}

Conversation

EgorBo commented Dec 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 9, 2023

Uh oh!

Uh oh!

am11 Dec 10, 2023 • 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!

EgorBo Dec 10, 2023

Choose a reason for hiding this comment

Uh oh!

am11 Dec 10, 2023

Choose a reason for hiding this comment

Uh oh!

EgorBo Dec 10, 2023

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakobbotsch Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Dec 11, 2023 • 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!

jakobbotsch Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented Dec 11, 2023

Uh oh!

EgorBo commented Apr 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EgorBo commented Dec 9, 2023 •

edited

Loading

am11 Dec 10, 2023 •

edited

Loading

EgorBo commented Dec 11, 2023 •

edited

Loading

jakobbotsch Dec 11, 2023 •

edited

Loading