Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve liveness for 'STORE_BLK(lcl_var)'. #48797

Merged

Conversation

sandreenko
Copy link
Contributor

The rule for an untracked variable was weaker so converting a tracked variable to untracked sometimes gave us better code.

Don't set lvMustInit for tracked lclVars that are compiler temps if they don't contain GC fields.

Tiny SPMI improvements on master but I need it for my future changes.
tests.pmi.windows.x64.checked.mch
3 total methods with Code Size differences (3 improved, 0 regressed), 0 unchanged.
Top method improvements (bytes):
          -7 (-3.61% of base) : 247502.dasm - TailCallOptTest:Caller2(System.Object,long,double,TypedDouble,TwoInts,TypedDouble):bool
          -7 (-3.37% of base) : 247500.dasm - TailCallOptTest:Caller1(System.Object,long,double,TypedDouble,TypedDouble,TypedDouble):bool
          -7 (-3.66% of base) : 249520.dasm - Program:Caller(int,int,int,int,MyStruct):int

libraries.pmi.windows.x64.checked.mch
1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged.
          -7 (-13.21% of base) : 7338.dasm - Microsoft.FSharp.Text.StructuredPrintfImpl.LayoutOps:boundedUnfoldL(Microsoft.FSharp.Core.FSharpFunc`2[__Canon,__Canon],Microsoft.FSharp.Core.FSharpFunc`2[Nullable`1,__Canon],Microsoft.FSharp.Core.FSharpFunc`2[Nullable`1,Boolean],System.Nullable`1[Int32],int):Microsoft.FSharp.Collections.FSharpList`1[[Microsoft.FSharp.Text.StructuredPrintfImpl.Layout, FSharp.Core, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]]

Diff example:
before.txt
after.txt

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 26, 2021
@sandreenko sandreenko added this to the 6.0.0 milestone Feb 26, 2021
@sandreenko sandreenko self-assigned this Feb 26, 2021
@sandreenko
Copy link
Contributor Author

@erozenfeld, could you please review it if you have time? It is not what I asked about but a different issue, the one that I asked I could not have fixed and will leave it for now.
PTAL @dotnet/jit-contrib,

Base automatically changed from master to main March 1, 2021 09:08
@sandreenko sandreenko force-pushed the enregStruct_part2_doNotTrackWhatCantEnreg branch from c5edbba to efb06ed Compare March 5, 2021 07:16
@sandreenko sandreenko changed the title Don't require init for tracked comp temps without GC. Improve liveness for 'STORE_BLK(lcl_var)'. Mar 5, 2021
@sandreenko
Copy link
Contributor Author

I have changed the solution to better analyse "store_blk" and use "store_lcl_var" in the cases where "store_blk" was used with no reason.

It gives improvements on the same set of methods, so the original commit was dropped, maybe I will still need in the future, with additional struct changes, but for now, it does not produce diffs.

The new diffs:

benchmarks.run.windows.x64.checked.mch
1 total methods with Code Size differences (1 improved, 0 regressed), 0 unchanged.
libraries.pmi.windows.x64.checked.mch
12 total methods with Code Size differences (12 improved, 0 regressed), 0 unchanged.
tests.pmi.windows.x64.checked.mch
3 total methods with Code Size differences (3 improved, 0 regressed), 0 unchanged

@sandreenko
Copy link
Contributor Author

Could somebody please review it @dotnet/jit-contrib ?

store->gtFlags |= GTF_VAR_DEF | GTF_ASG;
return store;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement

Copy link
Contributor

@briansull briansull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good

if (entire)
{
fgMarkUseDef(dummyLclVarTree);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good too.

While reviewing this change again I found that `fgMarkUseDef` should be called when we visit the local itself, the diffs that I saw from this change were because the LCL_VAR was not marked as `GTF_VAR_DEF`. Now the lclVar is marked as `GTF_VAR_DEF` so we don't need this extra logic.
@sandreenko sandreenko merged commit b38ea6a into dotnet:main Mar 10, 2021
@sandreenko sandreenko deleted the enregStruct_part2_doNotTrackWhatCantEnreg branch March 10, 2021 20:15
Copy link
Member

@erozenfeld erozenfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants