JIT: Generalize handling of commas in block morphing #83590

jakobbotsch · 2023-03-17T14:29:25Z

Eliminate commas early in block morphing, before the rest of the transformation
needs to look at it. Do this by extracting their side effects and adding
them on top of the returned result instead. This allows us to handle
arbitrary COMMAs on destination operand in a general manner.

Prerequisite to #83388.

Fix #1699.

ghost · 2023-03-17T14:29:38Z

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:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS

LGTM

SingleAccretion

We also have workarounds in place that can be deleted:

runtime/src/coreclr/jit/rationalize.cpp

Lines 72 to 73 in 6bcd4aa

    
           // TODO-ADDR: delete this once block morphing stops taking addresses of locals 
        
           // under COMMAs.

- pretty safe to just delete.

runtime/src/coreclr/jit/fginline.cpp

Lines 396 to 402 in 6bcd4aa

    
           // Block morphing does not support (promoted) locals under commas, as such, instead of "COMMA(asg, lcl)" we 
        
           // do "OBJ(COMMA(asg, ADDR(LCL)))". TODO-1stClassStructs: improve block morphing and delete this workaround. 
        
           // 
        
           GenTree* lcl  = m_compiler->gtNewLclvNode(lclNum, varDsc->TypeGet()); 
        
           GenTree* addr = m_compiler->gtNewOperNode(GT_ADDR, TYP_I_IMPL, lcl); 
        
           addr          = m_compiler->gtNewOperNode(GT_COMMA, addr->TypeGet(), asg, addr); 
        
           GenTree* obj  = m_compiler->gtNewObjNode(varDsc->GetLayout(), addr);

- this one has somewhat broad CQ impact so may not be as easy to delete.

Additionally, gtNewTempAssign calls impAssignStruct which tries to sink commas, which is then worked around in CSE and return merging. Notionally, it would be possible to get rid of the sinking.

Finally, this fixes #1699.

SingleAccretion · 2023-03-22T15:50:31Z

src/coreclr/jit/morphblock.cpp

    {
-        printf("%s (after):\n", GetHelperName());
-        m_comp->gtDispTree(m_result);
+        m_result = m_comp->gtNewOperNode(GT_COMMA, m_result->TypeGet(), sideEffects, m_result);


It's a bit unfortunate to recreate the comma nodes. I am assuming the memory consumption impact is negligible?

I did spend a while trying to avoid it, but it was significantly uglier to reuse the commas. Based on the throughput I think the case is very rare but let me double check.

I ended up reusing the commas by returning a pool of the freed ones from EliminateCommas, also linked via their gtNext fields. I couldn't find a more elegant way of doing this.

SingleAccretion · 2023-03-22T15:51:14Z

src/coreclr/jit/morphblock.cpp

+//   the created commas. Therefore this function just returns a linked list of
+//   the side effects to be used for that purpose.
+//
+GenTree* MorphInitBlockHelper::EliminateCommas()


What if the assignment was reversed?

Yep, I need to handle that.

SingleAccretion · 2023-03-22T15:53:15Z

src/coreclr/jit/morphblock.cpp

+        lhs = lhs->gtGetOp2();
+    }
+
+    assert(lhs->OperIsUnary() || lhs->OperIsLeaf());


OperIsUnary

Why not OperIsIndir (also below)?

SingleAccretion · 2023-03-22T16:13:31Z

src/coreclr/jit/morphblock.cpp

+    assert(lhs->OperIsUnary() || lhs->OperIsLeaf());
+
+    GenTree* rhs = m_asg->gtGetOp2();
+    if (lhs->OperIsUnary() && ((lhs->gtGetOp1()->gtFlags & GTF_ALL_EFFECT) != 0) && rhs->OperIs(GT_COMMA))


It does not look like the case of RHS comma containing an assignment that affects locals on the LHS is handled:

V00 = oldAddr // *(oldAddr + 4) = *(newAddr + 4) ASG IND(V00 + 4) COMMA ASG(V00 = newAddr) IND(V00 + 4) => // *(newAddr + 4) = *(newAddr + 4) COMMA ASG(V00 = newAddr) ASG IND(V00 + 4) IND(V00 + 4)

Good catch.

jakobbotsch · 2023-03-24T11:47:30Z

Updated this to address feedback. It's currently based on #83814 that will need to go in first.

pretty safe to just delete.

Thanks! Deleted this one.

this one has somewhat broad CQ impact so may not be as easy to delete.

Updated this too, overall an improvement (for win-arm64 at least) and diffs aren't that big.

Additionally, gtNewTempAssign calls impAssignStruct which tries to sink commas, which is then worked around in CSE and return merging. Notionally, it would be possible to get rid of the sinking.

I will leave this as-is for now, the function has some recursive handling right now and it probably would need to be restructured a bit.

Avoid creating commas that are on the LHS of an assignment or below an ADDR node. These primarily were created by morph which has an IND(COMMA(x, y)) -> COMMA(x, IND(y)) transformation that did not pay attention to whether it was on the left of an assignment. The IR shape is pretty odd; the RHS of these commas are not actually evaluated in the traditional sense, but happen as part of the parent ASG, so the semantics of the construct ends up being confusing. Additionally it complicates #83590.

Eliminate commas early in block morphing, before the rest of the transformation needs to look at it. Do this by extracting their side effects and adding them on top of the returned result instead. This allows us to handle arbitrary COMMAs on destination operand in a general manner. Prerequisite to dotnet#83388. Fix dotnet#1699.

jakobbotsch · 2023-03-27T11:42:37Z

@SingleAccretion Was RewriteSIMDIndir meant to be for CQ or correctness? We can still see IND(LCL_VAR_ADDR) nodes due to forward sub, and I am hitting an assert on this in the backend -- apparently we mark the LCL_VAR_ADDR as contained and emitHandleMemOp is not prepared to handle that.

jakobbotsch · 2023-03-27T13:48:24Z

I reverted the deletion of RewriteSIMDIndir, the backend needs to be taught how to handle IND<simd12>(LCL_VAR_ADDR/LCL_FLD_ADDR) with a contained address before it can be deleted. I will open an issue after this is merged.

SingleAccretion · 2023-03-27T14:05:00Z

It was meant to be for CQ only (at this point anyway, in the past it was also needed for correctness). Unfortunate that a bug has snuck in the meantime.

jakobbotsch · 2023-03-27T17:49:45Z

Diffs.

Can you please take another look @AndyAyersMS @SingleAccretion?

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 17, 2023

ghost assigned jakobbotsch Mar 17, 2023

jakobbotsch force-pushed the block-morph-commas branch 2 times, most recently from bef0cbb to 15ba192 Compare March 21, 2023 18:21

jakobbotsch changed the title ~~JIT: Eliminate commas early in block morphing~~ JIT: Generalize handling of commas in block morphing Mar 22, 2023

jakobbotsch marked this pull request as ready for review March 22, 2023 09:54

This comment was marked as outdated.

Sign in to view

jakobbotsch requested review from SingleAccretion and AndyAyersMS March 22, 2023 10:28

AndyAyersMS approved these changes Mar 22, 2023

View reviewed changes

SingleAccretion reviewed Mar 22, 2023

View reviewed changes

jakobbotsch mentioned this pull request Mar 23, 2023

JIT: Stop creating "location" commas #83814

Merged

jakobbotsch marked this pull request as draft March 24, 2023 10:35

build-analysis bot mentioned this pull request Mar 24, 2023

WasmTestOnBrowser-System.* test failures in CI #83655

Closed

jakobbotsch force-pushed the block-morph-commas branch from 659732b to 4512bf1 Compare March 25, 2023 11:39

Run jit-format

e251b2f

Revert RewriteSIMDIndir removal

e6fb448

Reset flags on commas taken from pool

87aed31

jakobbotsch marked this pull request as ready for review March 27, 2023 17:49

jakobbotsch requested review from SingleAccretion and AndyAyersMS March 27, 2023 17:49

SingleAccretion approved these changes Mar 27, 2023

View reviewed changes

AndyAyersMS approved these changes Mar 27, 2023

View reviewed changes

jakobbotsch merged commit eae7caa into dotnet:main Mar 28, 2023

jakobbotsch deleted the block-morph-commas branch March 28, 2023 07:03

AndyAyersMS mentioned this pull request Mar 30, 2023

[Perf] Linux/arm64: 6 Improvements on 3/28/2023 10:24:48 AM dotnet/perf-autofiling-issues#14668

Closed

jakobbotsch mentioned this pull request Mar 31, 2023

JIT: Add a (disabled) prototype for a generalized promotion pass #83388

Merged

This was referenced Apr 1, 2023

[Perf] Linux/arm64: 5 Improvements on 3/28/2023 10:24:48 AM dotnet/perf-autofiling-issues#14644

Closed

[Perf] Windows/arm64: 3 Improvements on 3/28/2023 10:24:48 AM dotnet/perf-autofiling-issues#14675

Closed

ghost locked as resolved and limited conversation to collaborators Apr 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Generalize handling of commas in block morphing #83590

JIT: Generalize handling of commas in block morphing #83590

jakobbotsch commented Mar 17, 2023 •

edited

Loading

ghost commented Mar 17, 2023

This comment was marked as outdated.

AndyAyersMS left a comment

SingleAccretion left a comment •

edited

Loading

SingleAccretion Mar 22, 2023

jakobbotsch Mar 22, 2023

jakobbotsch Mar 24, 2023

SingleAccretion Mar 22, 2023

jakobbotsch Mar 22, 2023

SingleAccretion Mar 22, 2023

SingleAccretion Mar 22, 2023

jakobbotsch Mar 22, 2023

jakobbotsch commented Mar 24, 2023 •

edited

Loading

jakobbotsch commented Mar 27, 2023

jakobbotsch commented Mar 27, 2023

SingleAccretion commented Mar 27, 2023

jakobbotsch commented Mar 27, 2023

	// TODO-ADDR: delete this once block morphing stops taking addresses of locals
	// under COMMAs.

	// Block morphing does not support (promoted) locals under commas, as such, instead of "COMMA(asg, lcl)" we
	// do "OBJ(COMMA(asg, ADDR(LCL)))". TODO-1stClassStructs: improve block morphing and delete this workaround.
	//
	GenTree* lcl = m_compiler->gtNewLclvNode(lclNum, varDsc->TypeGet());
	GenTree* addr = m_compiler->gtNewOperNode(GT_ADDR, TYP_I_IMPL, lcl);
	addr = m_compiler->gtNewOperNode(GT_COMMA, addr->TypeGet(), asg, addr);
	GenTree* obj = m_compiler->gtNewObjNode(varDsc->GetLayout(), addr);

JIT: Generalize handling of commas in block morphing #83590

JIT: Generalize handling of commas in block morphing #83590

Conversation

jakobbotsch commented Mar 17, 2023 • edited Loading

ghost commented Mar 17, 2023

This comment was marked as outdated.

AndyAyersMS left a comment

Choose a reason for hiding this comment

SingleAccretion left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Mar 24, 2023 • edited Loading

jakobbotsch commented Mar 27, 2023

jakobbotsch commented Mar 27, 2023

SingleAccretion commented Mar 27, 2023

jakobbotsch commented Mar 27, 2023

jakobbotsch commented Mar 17, 2023 •

edited

Loading

SingleAccretion left a comment •

edited

Loading

jakobbotsch commented Mar 24, 2023 •

edited

Loading