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

Scale cloned loop block weights #51901

Merged
merged 2 commits into from
Apr 27, 2021

Conversation

BruceForstall
Copy link
Member

The slow path of cloned loops are scaled to keep only 1% of the
original block weights, while the original loop keeps 99% of the weight.
It could be argued that the cloned loop slow path, as currently implemented,
should only be executed during exceptional paths, and thus should be
marked "run rarely", but don't do that for now.

There are many diffs, due to weights changing. Mostly, fewer CSEs in cloned
loop slow paths. E.g., aspnet spmi diffs included below.

Also, add a header comment for optRedirectBlock (previous change follow-up).
In addition, I didn't like the semantics of the addPreds flag -- it didn't
match well with the rest of the function -- so I removed it and added the appropriate
changes in optCloneLoop (the only caller that used that) to compensate.

I added code to optEnsureUniqueHead, only used by loop cloning, to add and update
appropriate preds edges. It turns out, however, this function is currently dead
code, due to other limitations in loop cloning. I'll leave the changes anyway, in
case those limitations are removed later. This part of the change I verified was no-diff
(and verified the code wasn't even executed in our SPMI collections).


Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 18811
Total bytes of diff: 18749
Total bytes of delta: -62 (-0.33% of base)
    diff is an improvement.
Detail diffs

Top file regressions (bytes):
          23 : 19019.dasm (9.91% of base)
          23 : 38588.dasm (9.91% of base)
           8 : 39269.dasm (1.15% of base)
           3 : 19897.dasm (0.94% of base)
           2 : 40501.dasm (1.43% of base)

Top file improvements (bytes):
         -37 : 13173.dasm (-2.41% of base)
         -37 : 14302.dasm (-2.41% of base)
         -17 : 31284.dasm (-0.84% of base)
         -17 : 34351.dasm (-0.83% of base)
          -5 : 31157.dasm (-0.25% of base)
          -5 : 34245.dasm (-0.25% of base)
          -3 : 38167.dasm (-0.49% of base)

12 total files with Code Size differences (7 improved, 5 regressed), 4 unchanged.

Top method regressions (bytes):
          23 ( 9.91% of base) : 19019.dasm - ObjectEqualityComparer`1:IndexOf(ref,__Canon,int,int):int:this
          23 ( 9.91% of base) : 38588.dasm - ObjectEqualityComparer`1:IndexOf(ref,__Canon,int,int):int:this
           8 ( 1.15% of base) : 39269.dasm - RuntimeMethodInfo:MakeGenericMethod(ref):MethodInfo:this
           3 ( 0.94% of base) : 19897.dasm - ControllerActionInvoker:PrepareArguments(IDictionary`2,ObjectMethodExecutor):ref
           2 ( 1.43% of base) : 40501.dasm - SslCredKey:Equals(SslCredKey):bool:this

Top method improvements (bytes):
         -37 (-2.41% of base) : 13173.dasm - DefaultTypeMap:FindConstructor(ref,ref):ConstructorInfo:this
         -37 (-2.41% of base) : 14302.dasm - DefaultTypeMap:FindConstructor(ref,ref):ConstructorInfo:this
         -17 (-0.84% of base) : 31284.dasm - EntityMaterializerInjectingExpressionVisitor:MaterializeEntity(EntityShaperExpression,ParameterExpression,ParameterExpression,ParameterExpression,ParameterExpression):Expression:this
         -17 (-0.83% of base) : 34351.dasm - EntityMaterializerInjectingExpressionVisitor:MaterializeEntity(EntityShaperExpression,ParameterExpression,ParameterExpression,ParameterExpression,ParameterExpression):Expression:this
          -5 (-0.25% of base) : 31157.dasm - EntityShaperExpression:GenerateMaterializationCondition(IEntityType,bool):LambdaExpression:this
          -5 (-0.25% of base) : 34245.dasm - EntityShaperExpression:GenerateMaterializationCondition(IEntityType,bool):LambdaExpression:this
          -3 (-0.49% of base) : 38167.dasm - RuntimeMethodInfo:Equals(Object):bool:this

Top method regressions (percentages):
          23 ( 9.91% of base) : 19019.dasm - ObjectEqualityComparer`1:IndexOf(ref,__Canon,int,int):int:this
          23 ( 9.91% of base) : 38588.dasm - ObjectEqualityComparer`1:IndexOf(ref,__Canon,int,int):int:this
           2 ( 1.43% of base) : 40501.dasm - SslCredKey:Equals(SslCredKey):bool:this
           8 ( 1.15% of base) : 39269.dasm - RuntimeMethodInfo:MakeGenericMethod(ref):MethodInfo:this
           3 ( 0.94% of base) : 19897.dasm - ControllerActionInvoker:PrepareArguments(IDictionary`2,ObjectMethodExecutor):ref

Top method improvements (percentages):
         -37 (-2.41% of base) : 13173.dasm - DefaultTypeMap:FindConstructor(ref,ref):ConstructorInfo:this
         -37 (-2.41% of base) : 14302.dasm - DefaultTypeMap:FindConstructor(ref,ref):ConstructorInfo:this
         -17 (-0.84% of base) : 31284.dasm - EntityMaterializerInjectingExpressionVisitor:MaterializeEntity(EntityShaperExpression,ParameterExpression,ParameterExpression,ParameterExpression,ParameterExpression):Expression:this
         -17 (-0.83% of base) : 34351.dasm - EntityMaterializerInjectingExpressionVisitor:MaterializeEntity(EntityShaperExpression,ParameterExpression,ParameterExpression,ParameterExpression,ParameterExpression):Expression:this
          -3 (-0.49% of base) : 38167.dasm - RuntimeMethodInfo:Equals(Object):bool:this
          -5 (-0.25% of base) : 31157.dasm - EntityShaperExpression:GenerateMaterializationCondition(IEntityType,bool):LambdaExpression:this
          -5 (-0.25% of base) : 34245.dasm - EntityShaperExpression:GenerateMaterializationCondition(IEntityType,bool):LambdaExpression:this

12 total methods with Code Size differences (7 improved, 5 regressed), 4 unchanged.


The slow path of cloned loops are scaled to keep only 1% of the
original block weights, while the original loop keeps 99% of the weight.
It could be argued that the cloned loop slow path, as currently implemented,
should only be executed during exceptional paths, and thus should be
marked "run rarely", but don't do that for now.

There are many diffs, due to weights changing. Mostly, fewer CSEs in cloned
loop slow paths. E.g., aspnet spmi diffs included below.

Also, add a header comment for `optRedirectBlock` (previous change follow-up).
In addition, I didn't like the semantics of the `addPreds` flag -- it didn't
match well with the rest of the function -- so I removed it and added the appropriate
changes in `optCloneLoop` (the only caller that used that) to compensate.

I added code to `optEnsureUniqueHead`, only used by loop cloning, to add and update
appropriate preds edges. It turns out, however, this function is currently dead
code, due to other limitations in loop cloning. I'll leave the changes anyway, in
case those limitations are removed later. This part of the change I verified was no-diff
(and verified the code wasn't even executed in our SPMI collections).

```

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 18811
Total bytes of diff: 18749
Total bytes of delta: -62 (-0.33% of base)
    diff is an improvement.
```
<details>

<summary>Detail diffs</summary>

```

Top file regressions (bytes):
          23 : 19019.dasm (9.91% of base)
          23 : 38588.dasm (9.91% of base)
           8 : 39269.dasm (1.15% of base)
           3 : 19897.dasm (0.94% of base)
           2 : 40501.dasm (1.43% of base)

Top file improvements (bytes):
         -37 : 13173.dasm (-2.41% of base)
         -37 : 14302.dasm (-2.41% of base)
         -17 : 31284.dasm (-0.84% of base)
         -17 : 34351.dasm (-0.83% of base)
          -5 : 31157.dasm (-0.25% of base)
          -5 : 34245.dasm (-0.25% of base)
          -3 : 38167.dasm (-0.49% of base)

12 total files with Code Size differences (7 improved, 5 regressed), 4 unchanged.

Top method regressions (bytes):
          23 ( 9.91% of base) : 19019.dasm - ObjectEqualityComparer`1:IndexOf(ref,__Canon,int,int):int:this
          23 ( 9.91% of base) : 38588.dasm - ObjectEqualityComparer`1:IndexOf(ref,__Canon,int,int):int:this
           8 ( 1.15% of base) : 39269.dasm - RuntimeMethodInfo:MakeGenericMethod(ref):MethodInfo:this
           3 ( 0.94% of base) : 19897.dasm - ControllerActionInvoker:PrepareArguments(IDictionary`2,ObjectMethodExecutor):ref
           2 ( 1.43% of base) : 40501.dasm - SslCredKey:Equals(SslCredKey):bool:this

Top method improvements (bytes):
         -37 (-2.41% of base) : 13173.dasm - DefaultTypeMap:FindConstructor(ref,ref):ConstructorInfo:this
         -37 (-2.41% of base) : 14302.dasm - DefaultTypeMap:FindConstructor(ref,ref):ConstructorInfo:this
         -17 (-0.84% of base) : 31284.dasm - EntityMaterializerInjectingExpressionVisitor:MaterializeEntity(EntityShaperExpression,ParameterExpression,ParameterExpression,ParameterExpression,ParameterExpression):Expression:this
         -17 (-0.83% of base) : 34351.dasm - EntityMaterializerInjectingExpressionVisitor:MaterializeEntity(EntityShaperExpression,ParameterExpression,ParameterExpression,ParameterExpression,ParameterExpression):Expression:this
          -5 (-0.25% of base) : 31157.dasm - EntityShaperExpression:GenerateMaterializationCondition(IEntityType,bool):LambdaExpression:this
          -5 (-0.25% of base) : 34245.dasm - EntityShaperExpression:GenerateMaterializationCondition(IEntityType,bool):LambdaExpression:this
          -3 (-0.49% of base) : 38167.dasm - RuntimeMethodInfo:Equals(Object):bool:this

Top method regressions (percentages):
          23 ( 9.91% of base) : 19019.dasm - ObjectEqualityComparer`1:IndexOf(ref,__Canon,int,int):int:this
          23 ( 9.91% of base) : 38588.dasm - ObjectEqualityComparer`1:IndexOf(ref,__Canon,int,int):int:this
           2 ( 1.43% of base) : 40501.dasm - SslCredKey:Equals(SslCredKey):bool:this
           8 ( 1.15% of base) : 39269.dasm - RuntimeMethodInfo:MakeGenericMethod(ref):MethodInfo:this
           3 ( 0.94% of base) : 19897.dasm - ControllerActionInvoker:PrepareArguments(IDictionary`2,ObjectMethodExecutor):ref

Top method improvements (percentages):
         -37 (-2.41% of base) : 13173.dasm - DefaultTypeMap:FindConstructor(ref,ref):ConstructorInfo:this
         -37 (-2.41% of base) : 14302.dasm - DefaultTypeMap:FindConstructor(ref,ref):ConstructorInfo:this
         -17 (-0.84% of base) : 31284.dasm - EntityMaterializerInjectingExpressionVisitor:MaterializeEntity(EntityShaperExpression,ParameterExpression,ParameterExpression,ParameterExpression,ParameterExpression):Expression:this
         -17 (-0.83% of base) : 34351.dasm - EntityMaterializerInjectingExpressionVisitor:MaterializeEntity(EntityShaperExpression,ParameterExpression,ParameterExpression,ParameterExpression,ParameterExpression):Expression:this
          -3 (-0.49% of base) : 38167.dasm - RuntimeMethodInfo:Equals(Object):bool:this
          -5 (-0.25% of base) : 31157.dasm - EntityShaperExpression:GenerateMaterializationCondition(IEntityType,bool):LambdaExpression:this
          -5 (-0.25% of base) : 34245.dasm - EntityShaperExpression:GenerateMaterializationCondition(IEntityType,bool):LambdaExpression:this

12 total methods with Code Size differences (7 improved, 5 regressed), 4 unchanged.

```

</details>

--------------------------------------------------------------------------------
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 27, 2021
@BruceForstall
Copy link
Member Author

@AndyAyersMS @dotnet/jit-contrib PTAL

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

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