Skip to content

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Apr 1, 2023

At this point, both GT_BLK and GT_OBJ represent struct loads and are essentially identical in purpose (if not functionality). This change unifies the two as GT_BLK, though I am naturally open to going the other way or choosing a different oper name altogether.

There will be a follow-up which similarly unifies STORE_BLK with STORE_OBJ.

Diffs: one CSE due to the change in gtSetEvalOrder.

A bit of a TP regression on 64 bit because MSVC doesn't inline gtIsLikelyRegVar in the same function, and some perturbance in the switch of fgComputeLifeLIR.

TP breakdown
Base: 1387501711, Diff: 1387934487, +0.0312%
                                                                                                                                                     
Compiler::gtIsLikelyRegVar                                                                        : 487075  : +36.55%  : 41.38% : +0.0351%
Compiler::fgComputeLifeLIR                                                                        : 250001  : +4.47%   : 21.24% : +0.0180%
Compiler::gtNewBlkIndir                                                                           : 32372   : NA       : 2.75%  : +0.0023%
SsaBuilder::RenameDef                                                                             : 10359   : +0.62%   : 0.88%  : +0.0007%
GenTree::OperExceptions                                                                           : 6980    : +0.13%   : 0.59%  : +0.0005%
Compiler::typGetObjLayout                                                                         : 6477    : +59.39%  : 0.55%  : +0.0005%
Compiler::gtNewIndexIndir                                                                         : 2971    : +58.90%  : 0.25%  : +0.0002%
Compiler::optRedundantBranch                                                                      : 2222    : +0.02%   : 0.19%  : +0.0002%
ValueNumStore::VNMap<ValueNumStore::VNDefFuncApp<2>,ValueNumStore::VNDefFuncAppKeyFuncs<2> >::Set : 1563    : +0.03%   : 0.13%  : +0.0001%
Lowering::LowerBlockStore                                                                         : -1420   : -3.77%   : 0.12%  : -0.0001%
ValueNumStore::VNForMapSelectWork                                                                 : -1431   : -0.06%   : 0.12%  : -0.0001%
Compiler::fgMorphSmpOp                                                                            : -3403   : -0.01%   : 0.29%  : -0.0002%
BitSetOps<unsigned __int64 *,1,Compiler *,TrackedVarBitSetTraits>::AssignAllowUninitRhs           : -5127   : -2.37%   : 0.44%  : -0.0004%
Compiler::gtNewStructVal                                                                          : -6541   : -20.75%  : 0.56%  : -0.0005%
ValueNumStore::VNForFuncNoFolding                                                                 : -7804   : -0.13%   : 0.66%  : -0.0006%
ValueNumStore::VNForFunc                                                                          : -10502  : -0.06%   : 0.89%  : -0.0008%
Compiler::fgPerBlockLocalVarLiveness                                                              : -18488  : -0.23%   : 1.57%  : -0.0013%
Compiler::fgValueNumberTree                                                                       : -22890  : -0.23%   : 1.94%  : -0.0016%
Compiler::gtNewObjNode                                                                            : -33337  : -100.00% : 2.83%  : -0.0024%
Compiler::gtSetEvalOrder                                                                          : -257867 : -1.05%   : 21.91% : -0.0186%

@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Apr 1, 2023
@ghost
Copy link

ghost commented Apr 1, 2023

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

Issue Details

At this point, both GT_BLK and GT_OBJ represent struct loads and are essentially identical in purpose (if not functionality). This change unified the two as GT_BLK, though I am naturally open to going the other way or choosing a different oper name altogether.

There will be a follow-up which similarly unifies STORE_BLK with STORE_OBJ.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SingleAccretion SingleAccretion marked this pull request as ready for review April 2, 2023 17:03
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@SingleAccretion SingleAccretion force-pushed the No-Obj-Upstream branch 3 times, most recently from 983db5d to 6ba7292 Compare April 3, 2023 16:38
@SingleAccretion SingleAccretion marked this pull request as draft April 3, 2023 20:27
GT_OBJ and GT_OBJ both represent struct loads. There is no need to have two.

Delete GT_OBJ as the more derived one.
@SingleAccretion SingleAccretion marked this pull request as ready for review April 3, 2023 20:35
@AndyAyersMS
Copy link
Member

@SingleAccretion can you resolve the conflicts?

@AndyAyersMS
Copy link
Member

@dotnet/jit-contrib anyone planning on reviewing this one?

I will take a look if there are not volunteers.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

LGTM.

blkNode->gtFlags |= indirFlags;
blkNode->SetIndirExceptionFlags(this);

// TODO-Bug: this method does not have enough information to make this determination.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a follow-up issue for this?

Copy link
Member

Choose a reason for hiding this comment

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

I see that comment just migrated here from the former OBJ case.

@AndyAyersMS AndyAyersMS merged commit e0120f0 into dotnet:main Apr 11, 2023
clamp03 added a commit to clamp03/runtime that referenced this pull request Apr 11, 2023
jakobbotsch pushed a commit that referenced this pull request Apr 11, 2023
@SingleAccretion SingleAccretion deleted the No-Obj-Upstream branch April 11, 2023 16:22
@ghost ghost locked as resolved and limited conversation to collaborators May 19, 2023
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 community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants