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.

Sign up for GitHub

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

Jump to bottom

Allow enregistering more structs args #39326

Merged

CarolEidt merged 5 commits into dotnet:master from CarolEidt:EnregStructArgs

Oct 7, 2020

Contributor

CarolEidt commented Jul 14, 2020

Allow HFAs and other structs with matching fields and registers.

Dotnet-GitSync-Bot added the area-CodeGen-coreclr label

CarolEidt mentioned this pull request

Struct, arg and float reg cleanup #39284

Merged

CarolEidt force-pushed the EnregStructArgs branch from 234c49f to 444c139 Compare

July 14, 2020 22:59

Contributor Author

CarolEidt commented Jul 14, 2020

This results in improvements primarily for Arm targets. Here are the crossgen diffs for frameworks & benchmarks (using altjits for x64UX, Arm and Arm64):

Target	Smaller	Larger	Size Diff	Faster	Slower	PerfScore Diff
X64Ux	4	0	-300 (-0.000%)	4	0	-91.10 (-0.000%)
Arm64	2409	17	-45880 (-0.044%)	2426	11	-18290.00 (-0.002%)
Arm32	300	6	-9848 (-0.014%)	301	6	-3762.80 (-0.000%)

The regressions result primarily from register allocation issues (we don't prefer callee-save for floating point registers, so promoting a floating point field is not always profitable if it's live across a call).

Contributor Author

CarolEidt commented Jul 14, 2020

@dotnet/jit-contrib PTAL
The first commit is #39284, which is still open (waiting for some test re-runs).

CarolEidt force-pushed the EnregStructArgs branch from 444c139 to 56ca049 Compare

July 15, 2020 04:47

sandreenko previously approved these changes

View reviewed changes

Contributor

sandreenko left a comment

It looks like a risky change, but I understand that it is better to get it into the Prev rather that an RC.
I have a few small question and hope you could run stress tests before the merge.
LGTM in case I don't wake up before the snap.

src/coreclr/src/jit/lclvars.cpp Outdated

    
                      {

                          canPromote = false;

                      }

              #ifdef UNIX_AMD64_ABI

              #if defined(FEATURE_SIMD) && defined(TARGET_ARMARCH)

                      // Don't promote a struct with a struct field unless it is the only field or an opaque SIMD type.

Contributor

sandreenko Jul 15, 2020

Nit: This is a perfect logical expression, but took me some time to check all cases. Maybe it could be rephrased a little bit? Maybe "A struct field that is not an opaque SIMD type blocks promotion if it it is not the only field." or delete the comment here and put it near !compiler->isOpaqueSIMDType(structPromotionInfo.fields[i].fldTypeHnd) saying that `opaque SIMD fields can be promoted'.

Contributor Author

CarolEidt Sep 29, 2020

This comment was reworded, hopefully it's more clear now.

src/coreclr/src/jit/lclvars.cpp

    
                              if (varTypeIsStruct(structPromotionInfo.fields[i].fldType) &&

                                  !compiler->isOpaqueSIMDType(structPromotionInfo.fields[i].fldTypeHnd))

                              {

                                  canPromote = false;

Contributor

sandreenko Jul 15, 2020

Does not it create aljit issues similar to #38980?

Contributor Author

CarolEidt Sep 29, 2020

It may, though I haven't run into them. That, however, is an altjit-specific issue so I don't think it should hold up this PR.

src/coreclr/src/jit/lclvars.cpp Show resolved Hide resolved

src/coreclr/src/jit/lclvars.cpp Outdated

    
              //

              // Arguments:

              //    lclNode - the GT_LCL_VAR node.

              //    varDsc - the local variable

Contributor

sandreenko Jul 15, 2020

Suggested change

      
            //    varDsc - the local variable
          
            //    varDsc - the local variable descriptor?

src/coreclr/src/jit/lclvars.cpp Outdated

    
                  else if (varDsc->lvIsStructField)

                  {

                      noway_assert(varDsc->lvParentLcl < lvaCount);

                      lvaTable[varDsc->lvParentLcl].lvStkOffs = varDsc->lvStkOffs;

Contributor

sandreenko Jul 15, 2020

Do we leave it set to BAD_STK_OFFS after this change?

Contributor Author

CarolEidt Sep 29, 2020

No, this was duplicative; the field offsets are set in the promoted struct case, and don't need to be set again when they are encountered.

src/coreclr/src/jit/morph.cpp Outdated

    
            @@ -17401,6 +17405,9 @@ void Compiler::fgPromoteStructs()
          
                  lvaStructPromotionInfo structPromotionInfo;

                  bool                   tooManyLocalsReported = false;

                  // Clear the structPromotionHelper, since it is conservative about looking up SIMD info.

Contributor

sandreenko Jul 15, 2020

I think I am not following it. Could you please explain? structPromotionHelper has structPromotionInfo that is a private temporary buffer that we use to keep information about the last analyzed type. We access it only in TryPromoteStructVar and nowhere else.
Where could we access structPromotionHelper->structPromotionInfo.typeHnd that is cleared here?

Contributor Author

CarolEidt Sep 29, 2020

The cache is used by the inliner, during which it is conservative about looking up SIMD info. I clarified the comment a bit.

src/coreclr/src/jit/codegencommon.cpp Outdated

    
                                      {

                                          int       nextArgNum = argNum + i;

                                          regNumber nextRegNum =

                                              genMapRegArgNumToRegNum(nextArgNum, regArgTab[nextArgNum].getRegType(compiler));

Contributor

sandreenko Jul 15, 2020

Nit: extract regArgTab[nextArgNum] to avoid the line break and use it in the next line as well.

jaredpar mentioned this pull request

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

sandreenko self-requested a review

July 23, 2020 07:59

sandreenko dismissed their stale review

July 23, 2020 08:00

outdated

CarolEidt mentioned this pull request

[ARM64] Performance regression: Sorting arrays of primitive types #41741

Closed

CarolEidt force-pushed the EnregStructArgs branch from cf4f903 to b67b73d Compare

September 22, 2020 18:00

CarolEidt force-pushed the EnregStructArgs branch from 7fbf6b0 to a05526c Compare

September 29, 2020 20:43

Contributor Author

CarolEidt commented Sep 29, 2020

@dotnet/jit-contrib PTAL
There are two new commits:

5592adf which fixes a failure on x64 Linux.
a05526c which addresses a previous PR feedback.

Prior to the latest push, I ran jitStressRegs, and there were some test build failures. There were no actual test failures, but having rebased, once the normal CI tests complete I'll run jitStressRegs again and hopefully the test build will succeed.

Contributor Author

CarolEidt commented Oct 1, 2020

ping @dotnet/jit-contrib
This has now passed both the regular CI and jitstressregs

Member

AndyAyersMS commented Oct 1, 2020 •

edited

Loading

Does this fix the lack of enregistration/promotion in InsertionSort seen over on #41741 (Span passed by-value)?

It would show up in PMI diffs, I think (for arm64).

sandreenko approved these changes

View reviewed changes

Contributor

sandreenko left a comment

LGTM

src/coreclr/src/jit/morph.cpp

    
            @@ -17503,6 +17511,11 @@ void Compiler::fgPromoteStructs()
          
                  lvaStructPromotionInfo structPromotionInfo;

                  bool                   tooManyLocalsReported = false;

                  // Clear the structPromotionHelper, since it is used during inlining, at which point it

                  // may be conservative about looking up SIMD info.

                  // We don't want to preserve those conservative decisions for the actual struct promotion.

Contributor

sandreenko Oct 1, 2020

Thanks for the explanation and the comment, I would probably rephrase it as: since it is used during importation for inlining decisions.

the place that uses it before we call struct promotion is:

runtime/src/coreclr/src/jit/importer.cpp

Lines 18651 to 18658 in 334bd84

    
           // Note if the callee's class is a promotable struct 
        
           if ((info.compClassAttr & CORINFO_FLG_VALUECLASS) != 0) 
        
           { 
        
               assert(structPromotionHelper != nullptr); 
        
               if (structPromotionHelper->CanPromoteStructType(info.compClassHnd)) 
        
               { 
        
                   inlineResult->Note(InlineObservation::CALLEE_CLASS_PROMOTABLE); 
        
               }

Contributor Author

CarolEidt commented Oct 2, 2020

@AndyAyersMS - this doesn't fix the InsertionSort case. For some reason the parameter is still marked do-not-enreg[SF] ld-addr-op; I haven't analyzed it to determine why.

Here are the diffs:

Arch	OS	What	Delta	Methods Improved	Methods Regressed
Arm64	Windows	Crossgen fx+tests	-6676 (-0.01%)	307	13
x64	Linux	Crossgen fx+tests	-1781 (-0.00%)	57	0
Arm64	Windows	PMI fx+benchmarks	-9952 (-0.02%)	447	4
x64	Linux	PMI fx+benchmarks	-3837 (-0.01%)	136	20

I'll have a look at InsertionSort before merging.

CarolEidt added 5 commits

October 6, 2020 09:41


          Allow enregistering more structs args

2c51383

Allow HFAs and other structs with matching fields and registers.

Contributes to dotnet#37924


          PR feedback and fix mixed type on x64/ux

69c19f2


          Fix for ARM HFA

b446edc


          Clear multi-reg flag on lclVar if source is not multi-reg.

7595c68


          Clarify comment

134dd48

CarolEidt force-pushed the EnregStructArgs branch from a05526c to 134dd48 Compare

October 6, 2020 16:47

Contributor Author

CarolEidt commented Oct 6, 2020

It turns out that these changes only support enregistering HFAs for Arm64; other multi-reg structs aren't promoted. I have a change that handles them, but will submit a separate PR for that.

CarolEidt merged commit 65a27aa into dotnet:master

CarolEidt mentioned this pull request

Enable promotion of structs passed in multiple registers with matching fields #37924

Closed

CarolEidt deleted the EnregStructArgs branch

October 7, 2020 18:35

CarolEidt mentioned this pull request

Pi-Digits: Extra Struct copies of BigInteger #5785

Closed

ghost locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr