StoreIND/Store_BLK/Store_OBJ improvements. #38316

sandreenko · 2020-06-24T01:36:55Z

This is a two parts change:

As usual, I recommend reviewing by commits, because there are several extracting/refactoring changes.

First part:

ceec9c8: Arm64: Support contained GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR under IND, STOREIND.

eefeb7e: XARCH: Support contained GT_LCL_FLD_ADDR under GT_STORE_IND, GT_IND.

ef2709d: Clear GTF_IND_ASG_LHS after Rationalize for STORE_OBJ/BLK.
We were doing this for STOREIND, but forgetting for STORE_OBJ/BLK.

eecb41a: Extract LowerStoreIndirCommon, LowerIndir.
Extract without changes, I will need to add additional calls to them later.

6f8575e: Call the extracted funtions.
Fix a few regressions with NoRetyping.

Second part:

d8821cc: Create LEA of comples addr expr.
Gives a few improvements when addr has an index.

f642505: Extract LowerBlockStoreCommon.

0624264: Tranform STORE_BLK/OBJ into STOREIND.
When it is possible and profitable.

428a86b: Don't tranform for GC and small types.
For GC types we were not trying to contain addr(why?) when needed a barrier and it looked dangerous for me to do such a change(for 5.0). @erozenfeld helped me to pass tests with GC types transformation, but I still was not confident enough and saw strange asm diffs like byref copy replaced by ref copy in some cases.

For small types we were generating movzx rax for IND byte instead of mov ax .

Diffs (sorry markdown doesn't understand merged word cells, so I have pasted it as an image)

I am planning to open two follow up issues if this goes in:

combine reuse reg and containment analysis, there are many methods that decide to mark CNT_INT int 0 as contained, but have a 0 available for reuse already (contained happens in lowering, reusing happens after in LSRA, a simple fix is to check reuse possibility even for contained, will fix xor rax, rax; mov byte ptr [esi], 0;, but not mov byte ptr [esi], 0; xor rax, rax;
STOREIND should always generate code not worse than STOREOBJ for the same copy block, right now it doesn't hold for GCRefs and small types.

I need these changes for NoRetyping, because there we have move structs and for STORE_LCL_VAR(src) when src is not
a LCL_VAR we are generating STORE_OBJ and it caused many regressions.

Support contained `GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR` under `IND, STOREIND`.

Support contained `GT_LCL_FLD_ADDR` under `GT_STORE_IND, GT_IND`.

We were doing this for `STOREIND`, but forgetting for `STORE_OBJ/BLK`.

Extract without changes, I will need to add additional calls to them later.

Fix a few regressions with `NoRetyping`.

Gives a few improvements when addr has an index.

When it is possible and profitable.

For GC types we were not trying to contain addr(why?) when needed a barrier and it looked dangerous for me to do such a change(for 5.0). For small types we were generating `movzx rax` for `IND byte` instead of `mov ax` .

sandreenko · 2020-06-25T21:43:02Z

PTAL @erozenfeld @dotnet/jit-contrib

CarolEidt

LGTM with minor suggested changes.

CarolEidt · 2020-06-26T18:23:57Z

src/coreclr/src/jit/rationalize.cpp

@@ -700,10 +700,14 @@ Compiler::fgWalkResult Rationalizer::RewriteNode(GenTree** useEdge, Compiler::Ge
        case GT_BLK:
            // We should only see GT_BLK for TYP_STRUCT or for InitBlocks.
            assert((node->TypeGet() == TYP_STRUCT) || use.User()->OperIsInitBlkOp());
+            // Clear the `GTF_IND_ASG_LHS` flag, which overlaps with `GTF_IND_REQ_ADDR_IN_REG`.
+            node->gtFlags &= ~GTF_IND_ASG_LHS;


I would recommend moving the GT_BLK and GT_OBJ cases next to the GT_IND cases, so the common characteristics are clearer. You might even consider extracting them all to a common method, e.g. RewriteIndir(GenTreeIndir* indir).

Thank you, fixed. I will need to do more changes there if I want to fix missed TGT_ANYWHERE flag, so it will be useful to have a separate method.

Excellent - thanks!

src/coreclr/src/jit/lower.cpp

CarolEidt · 2020-06-26T18:33:59Z

src/coreclr/src/jit/lower.h

    void LowerStoreIndir(GenTreeIndir* node);
    GenTree* LowerAdd(GenTreeOp* node);
    bool LowerUnsignedDivOrMod(GenTreeOp* divMod);
    GenTree* LowerConstIntDivOrMod(GenTree* node);
    GenTree* LowerSignedDivOrMod(GenTree* node);
    void LowerBlockStore(GenTreeBlk* blkNode);
+    void LowerBlockStoreCommon(GenTreeBlk* blkNode);


Unless I am missing something, these could be merged; I don't understand the reason for a separate method.

Do you mean LowerBlockStore and LowerBlockStoreCommon? The first is defined separately for armarch and xarch.

Oh I see - I missed that; thanks!

CarolEidt · 2020-06-26T18:37:59Z

src/coreclr/src/jit/lower.cpp

+    }
+    if (varTypeIsSIMD(regType))
+    {
+        // TODO: support STORE_IND SIMD16(SIMD16, CNT_INT 0).


This and the TODO below should probably be TODO-CQ

Co-authored-by: Carol Eidt <[email protected]>

CarolEidt

LGTM - thanks!

erozenfeld

LGTM.

I left a couple of minor comments.

sandreenko added arch-arm32 arch-arm64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Jun 24, 2020

sandreenko force-pushed the storeindImprovements branch from 3370f8f to 383144f Compare June 24, 2020 07:52

sandreenko added arch-x64 and removed arch-arm32 labels Jun 24, 2020

sandreenko force-pushed the storeindImprovements branch 2 times, most recently from 2325c40 to 29b2de5 Compare June 25, 2020 02:17

Sergey Andreenko added 9 commits June 25, 2020 13:35

Arm64: support contained GT_LCL_VAR/FLD_ADDR in indir.

ceec9c8

Support contained `GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR` under `IND, STOREIND`.

XARCH: Support contained GT_LCL_FLD_ADDR in IND.

eefeb7e

Support contained `GT_LCL_FLD_ADDR` under `GT_STORE_IND, GT_IND`.

Clear GTF_IND_ASG_LHS after Rationalize for STORE_OBJ/BLK.

ef2709d

We were doing this for `STOREIND`, but forgetting for `STORE_OBJ/BLK`.

Extract LowerStoreIndirCommon, LowerIndir.

eecb41a

Extract without changes, I will need to add additional calls to them later.

Call the extracted funtions.

6f8575e

Fix a few regressions with `NoRetyping`.

Create LEA of comples addr expr.

d8821cc

Gives a few improvements when addr has an index.

Extract LowerBlockStoreCommon.

f642505

Tranform STORE_BLK/OBJ into STOREIND.

0624264

When it is possible and profitable.

Don't tranform for GC and small types.

428a86b

For GC types we were not trying to contain addr(why?) when needed a barrier and it looked dangerous for me to do such a change(for 5.0). For small types we were generating `movzx rax` for `IND byte` instead of `mov ax` .

sandreenko force-pushed the storeindImprovements branch from 29b2de5 to 428a86b Compare June 25, 2020 21:35

sandreenko marked this pull request as ready for review June 25, 2020 21:43

CarolEidt suggested changes Jun 26, 2020

View reviewed changes

Sergey Andreenko and others added 3 commits June 26, 2020 11:44

Update src/coreclr/src/jit/lower.cpp

f02cb0c

Co-authored-by: Carol Eidt <[email protected]>

Use TODO-CQ.

8d59661

Extract Rationalizer::RewriteIndir.

52674ed

CarolEidt approved these changes Jun 26, 2020

View reviewed changes

erozenfeld approved these changes Jun 27, 2020

View reviewed changes

Fix code review.

ccb88f1

sandreenko merged commit a2f1b58 into dotnet:master Jun 28, 2020

This was referenced Jun 29, 2020

Use '0' from reg when it is available. #38535

Open

STOREIND and STORE_OBJ should generate the same code for the same copy. #38538

Open

sandreenko mentioned this pull request Jun 30, 2020

Disable JitDoOldStructRetyping by default. #37745

Merged

AndyAyersMS mentioned this pull request Jul 10, 2020

Microsoft.WindowsDesktop.App (latest runtime) 5.0.0-preview.8.20330.4 cannot display dotnet/wpf#3226

Closed

This was referenced Jul 10, 2020

Fix STOREIND optimization. #39066

Merged

Optimizations introduce null reference exception on latest preview 8 daily build #39403

Closed

sandreenko mentioned this pull request Jul 17, 2020

Fix lcl fld addr. #39424

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StoreIND/Store_BLK/Store_OBJ improvements. #38316

StoreIND/Store_BLK/Store_OBJ improvements. #38316

sandreenko commented Jun 24, 2020 •

edited

Loading

sandreenko commented Jun 25, 2020

CarolEidt left a comment

CarolEidt Jun 26, 2020

sandreenko Jun 26, 2020

CarolEidt Jun 26, 2020

CarolEidt Jun 26, 2020

sandreenko Jun 26, 2020

CarolEidt Jun 26, 2020

CarolEidt Jun 26, 2020

sandreenko Jun 26, 2020

CarolEidt left a comment

erozenfeld left a comment

StoreIND/Store_BLK/Store_OBJ improvements. #38316

StoreIND/Store_BLK/Store_OBJ improvements. #38316

Conversation

sandreenko commented Jun 24, 2020 • edited Loading

This is a two parts change:

First part:

Second part:

Diffs (sorry markdown doesn't understand merged word cells, so I have pasted it as an image)

sandreenko commented Jun 25, 2020

CarolEidt left a 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

Choose a reason for hiding this comment

CarolEidt left a comment

Choose a reason for hiding this comment

erozenfeld left a comment

Choose a reason for hiding this comment

sandreenko commented Jun 24, 2020 •

edited

Loading