Struct & SIMD improvements by CarolEidt · Pull Request #22255 · dotnet/coreclr

CarolEidt · 2019-01-28T22:00:16Z

Enable CSE of struct values when handle is available (and add code to get the handle of HW SIMD types)
Don't require block nodes for SIMD assignments
Don't set GTF_GLOB_REF on GT_OBJ if it is local
Set lvRegStruct on promoted SIMD fields
Add tests for #19910 (fixed with this PR) and #3539 & #19910 (fixed with Don't struct-promote opaque vectors #21314)
Additional cleanup

mikedn · 2019-01-28T22:12:37Z

-                           ArrayStack<GenTree*>& parents,
-                           CORINFO_METHOD_HANDLE callHnd,
+    void RewriteNodeAsCall(GenTree**               use,
+                           Compiler::GenTreeStack& parents,


Why the change?

Hmmm that change is presumably no longer relevant, since I can't see the need for it. At one point I believe I needed to call an existing method that took a GenTreeStack and the compiler didn't think that ArrayStack<GenTree*> was equivalent. In any case it probably makes sense for it to use the typedef'd type.

I'd keep ArrayStack<GenTree*>. It's not some convoluted template instantiation that warrants the use of a typedef and the GenTreeStack typedef is used only in 3 places so it's best not to add more (and remove it at some point).

AndyAyersMS · 2019-01-31T21:24:47Z

Any chance this can CSE the repeated use of Range.Start or Range.End in something like:

coreclr/src/System.Private.CoreLib/shared/System/Span.Fast.cs

Lines 171 to 179 in e7c6f87

    
           public Span<T> this[Range range] 
        
           { 
        
               get 
        
               { 
        
                   int start = range.Start.FromEnd ? _length - range.Start.Value : range.Start.Value; 
        
                   int end = range.End.FromEnd ? _length - range.End.Value : range.End.Value; 
        
                   return Slice(start, end - start); 
        
               } 
        
           }

mrange · 2019-03-19T21:52:55Z

👍 I really hope for this in dotnet 3.0 release.

CarolEidt · 2019-03-20T00:01:10Z

I really hope for this in dotnet 3.0 release.

Working on it, and hope to get it in.

- Enable CSE of struct values when handle is available (and add code to get the handle of HW SIMD types) - Don't require block nodes for SIMD assignments - Don't set `GTF_GLOB_REF` on `GT_OBJ` if it is local - Set `lvRegStruct` on promoted SIMD fields - Add tests for #19910 (fixed with this PR) and #3539 & #19438 (fixed with dotnet#21314) - Additional cleanup Fix #19910

CarolEidt · 2019-03-22T15:44:35Z

@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test

CarolEidt · 2019-03-22T15:45:52Z

@dotnet/jit-contrib PTAL

AndyAyersMS

Still looking this over, so may have more comments later.

I'm assuming this generates a fair number of diffs, and that you've done some tuning for CSE and similar already.

Can you start characterizing the diffs?

AndyAyersMS · 2019-03-22T17:55:25Z

+                                    CORINFO_FIELD_HANDLE fieldHnd = fieldSeq->m_fieldHnd;
+                                    CorInfoType fieldCorType = info.compCompHnd->getFieldType(fieldHnd, &structHnd);
+                                    assert(fieldCorType == CORINFO_TYPE_VALUECLASS);
+                                }


There is similar handle searching logic in gtGetClassHandle -- perhaps factor this out as a helper method?

They're really quite different, and though some logic could be sharable, the ref context seems quite different from the value-type context.

Can't we just get rid of this completly? It seems preferrable to keep GT_OBJ nodes around and extract the struct handle from those instead of attempting to recover handles from GT_IND this way.

I agree that we'd like to get rid of this in the long term, but this code is to deal with cases where we have already made that transformation. I'm going to look into this a bit further, so that I better understand the cases that require this.

Yes, my comment was targeted at long term. Well, maybe not that long, just only after I'm done with #21705 that makes GT_OBJ node "small" and overall cheap.

mikedn · 2019-03-22T18:54:30Z

+            noway_assert(asgType != TYP_STRUCT);
+            if (varTypeIsStruct(asgType))
+            {
+                destLclVarTree = fgIsIndirOfAddrOfLocal(dest);


Interesting, the OperIsBlk() branch above uses impIsAddressInLocal rather than fgIsIndirOfAddrOfLocal. I suspect that using impIsAddressInLocal there is actually incorrect but that's another story.

CarolEidt · 2019-03-22T21:43:29Z

I'm assuming this generates a fair number of diffs, and that you've done some tuning for CSE and similar already.

I did a small amount of tuning; more is probably warranted.

Can you start characterizing the diffs?

For x64 windows checked (frameworks + tests):

Total bytes of diff: -41406 (-0.04% of base)
4520 total methods with size differences (4236 improved, 284 regressed), 469768 unchanged.

Most of the diffs are due to not marking things as do-not-enregister and additional CSE's. The biggest impacts are in methods that use SIMD or vector types, but there are also some cases where we enregister promoted (non-SIMD) structs that were previously not enregistered.

There are some regressions (of which I looked at many, but not nearly all), due to CSE'd values that cause spill, extra callee-save reg save/restore in prolog/epilog, and a case where we're doing a tail call (local was previously marked address-exposed when it was only address-taken.

mikedn · 2019-03-24T08:52:47Z

-        break;
+            // Rewrite these as GT_IND.
+            assert(node->TypeGet() == TYP_STRUCT || !use.User()->OperIsInitBlkOp());
+            RewriteSIMDOperand(use, false);


I think this would be clearer if the call to RewriteSIMDOperand is wrapped in if (varTypeIsSIMD(node)) like in the GT_IND case. And the // Rewrite these as GT_IND. comment should be inside the if.

AndyAyersMS

Looked things over once again and everything I'd comment on seems covered.

CarolEidt · 2019-03-26T18:19:17Z

@dotnet-bot test Ubuntu x64 Checked CoreFX Tests

mmitche · 2019-03-26T21:40:52Z

@dotnet-bot test Windows_NT arm Cross Checked Innerloop Build and Test

mmitche · 2019-03-26T21:41:05Z

@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test

sandreenko

LGTM

CarolEidt · 2019-03-27T15:19:33Z

@dotnet-bot test Ubuntu arm Cross Checked crossgen_comparison Build and Test
@dotnet-bot test Ubuntu arm Cross Release crossgen_comparison Build and Test

CarolEidt · 2019-03-28T00:34:21Z

@dotnet/jit-contrib - any objections to merging this? I can't seem to re-trigger the "Test Pri0 Linux arm checked" that was "Cancelled" and I'm not optimistic that a re-try will address it.

AndyAyersMS · 2019-03-28T00:36:43Z

No objection...

mrange · 2019-03-28T18:32:55Z

Cool, how can I test this?

CarolEidt · 2019-03-28T21:02:39Z

@mrange - now that this is merged, you can build the coreclr repo and use it. (Instructions here: https://github.com/dotnet/coreclr/blob/master/Documentation/workflow/UsingYourBuild.md)
Or, you can download the daily (I think) build once this has been incorporated here: https://github.com/dotnet/core-sdk and some advice about finding the commit can be found here: https://github.com/dotnet/core-setup/blob/master/Documentation/project-docs/how-to-track-changes.md#what-git-commit-was-a-particular-binary-built-from
Ping me (or post an issue) if you find problems with the documentation.

CarolEidt · 2019-04-02T16:56:09Z

-                {
-                    // TODO-1stClassStructs: Previously, spillGlobEffects was set to true for
-                    // GT_INITBLK and GT_COPYBLK, but this is overly conservative, and should be
-                    // revisited. (Note that it was NOT set to true for GT_COPYOBJ.)


This was where we were previously always spilling on a non-COPYOBJ block assignment. This was (clearly) overly conservative, but by eliminating this conservatism, I failed to add the struct case to the code in impImportBlockCode (at the SPILL_APPEND: label in importer.cpp) (bug #23545, fixed in PR #23570)

* [WIP] Struct & SIMD improvements - Enable CSE of struct values when handle is available (and add code to get the handle of HW SIMD types) - Don't require block nodes for SIMD assignments - Don't set `GTF_GLOB_REF` on `GT_OBJ` if it is local - Set `lvRegStruct` on promoted SIMD fields - Add tests for dotnet/coreclr#19910 (fixed with this PR) and dotnet/coreclr#3539 & dotnet/coreclr#19438 (fixed with dotnet/coreclr#21314) - Additional cleanup Fix dotnet/coreclr#19910 Commit migrated from dotnet/coreclr@3d4a1d5

mikedn reviewed Jan 28, 2019

View reviewed changes

CarolEidt force-pushed the 1stClassStructToDo1 branch from c415cd2 to 2429f7b Compare March 19, 2019 16:55

CarolEidt force-pushed the 1stClassStructToDo1 branch from 2429f7b to 3d35992 Compare March 21, 2019 00:26

Fix Test

a706573

CarolEidt changed the title ~~[WIP] Struct & SIMD improvements~~ Struct & SIMD improvements Mar 22, 2019

AndyAyersMS reviewed Mar 22, 2019

View reviewed changes

mikedn reviewed Mar 22, 2019

View reviewed changes

mikedn reviewed Mar 24, 2019

View reviewed changes

AndyAyersMS approved these changes Mar 26, 2019

View reviewed changes

PR Feedback

18da24d

CarolEidt closed this Mar 26, 2019

CarolEidt reopened this Mar 26, 2019

sandreenko reviewed Mar 26, 2019

View reviewed changes

Comment thread src/jit/liveness.cpp

sandreenko reviewed Mar 26, 2019

View reviewed changes

Comment thread src/jit/morph.cpp

CarolEidt closed this Mar 26, 2019

CarolEidt reopened this Mar 26, 2019

Clarify fgRemoveDeadStore

9264123

sandreenko approved these changes Mar 26, 2019

View reviewed changes

CarolEidt merged commit 3d4a1d5 into dotnet:master Mar 28, 2019

echesakov mentioned this pull request Mar 28, 2019

Restrict Ubuntu.1404.Arm32.Open from running against PRs #23523

Merged

CarolEidt deleted the 1stClassStructToDo1 branch March 28, 2019 21:02

jkotas mentioned this pull request Mar 29, 2019

[master] Update dependencies from dotnet/coreclr dotnet/corefx#36480

Closed

CarolEidt commented Apr 2, 2019

View reviewed changes

briansull mentioned this pull request Apr 5, 2019

Block the hoisting of TYP_STRUCT rvalues in loop hoisting #23739

Merged

AndyAyersMS mentioned this pull request Dec 11, 2019

JIT optimization removes some necessary code in .NET Core 3.0/3.1 dotnet/runtime#764

Closed

tannergooding mentioned this pull request May 18, 2020

Assertion failed 'varDsc->lvExactSize == 12' dotnet/runtime#36586

Closed

Conversation

CarolEidt commented Jan 28, 2019

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!

AndyAyersMS commented Jan 31, 2019

Uh oh!

mrange commented Mar 19, 2019

Uh oh!

CarolEidt commented Mar 20, 2019

Uh oh!

CarolEidt commented Mar 22, 2019

Uh oh!

CarolEidt commented Mar 22, 2019

Uh oh!

AndyAyersMS left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarolEidt commented Mar 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

CarolEidt commented Mar 26, 2019

Uh oh!

Uh oh!

Uh oh!

mmitche commented Mar 26, 2019

Uh oh!

mmitche commented Mar 26, 2019

Uh oh!

sandreenko left a comment

Choose a reason for hiding this comment

Uh oh!

CarolEidt commented Mar 27, 2019

Uh oh!

CarolEidt commented Mar 28, 2019

Uh oh!

AndyAyersMS commented Mar 28, 2019

Uh oh!

mrange commented Mar 28, 2019

Uh oh!

CarolEidt commented Mar 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants