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

Transform STRUCT-typed uses of primitives in local morph #78131

Merged
merged 8 commits into from
Nov 30, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Nov 9, 2022

And update global morph accordingly. This mainly involves not DNER-ing struct local fields right away, but waiting for global morph to do this, after it has had a chance to transform the field into an LCL_VAR.

Diffs are a bit mixed, due to requiring the substitution workaround but generally positive, and overall minor. The larger regressions in tests are due to us setting some DNERs later, thus expanding things into field-by-field copies more.

Note: this change is best reviewed with the "don't show whitespace diffs" setting.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 9, 2022
@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 Nov 9, 2022
@ghost
Copy link

ghost commented Nov 9, 2022

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

Issue Details

And update global morph accordingly. This mainly involves giving not DNER-ing local fields right away, but waiting for global morph to do this, after it has had a chance to transform the field into an LCL_VAR.

Diffs are a bit mixed, due to requiring the substitution workaround but generally positive, and overall minor.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SingleAccretion SingleAccretion force-pushed the LclMorph-StructUse-Upstream branch from 0c36c00 to c911fd2 Compare November 9, 2022 21:33
@SingleAccretion
Copy link
Contributor Author

This now depends on #76491.

@SingleAccretion SingleAccretion force-pushed the LclMorph-StructUse-Upstream branch from f532bc1 to 733af07 Compare November 10, 2022 17:02
@SingleAccretion SingleAccretion force-pushed the LclMorph-StructUse-Upstream branch from 733af07 to c4fe2f5 Compare November 11, 2022 13:40
@SingleAccretion SingleAccretion force-pushed the LclMorph-StructUse-Upstream branch from c4fe2f5 to 320cb8a Compare November 11, 2022 17:09
@SingleAccretion SingleAccretion marked this pull request as ready for review November 11, 2022 21:16
@SingleAccretion
Copy link
Contributor Author

This is ready for review, but still depends on #76491.

@dotnet/jit-contrib

@BruceForstall
Copy link
Member

@SingleAccretion Needs conflict resolution

@SingleAccretion
Copy link
Contributor Author

Needs conflict resolution

Also done.

@jakobbotsch
Copy link
Member

/azp run runtime-coreclr superpmi-diffs, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Nov 29, 2022

Libraries stress failures are #78909 and #78912.

@SingleAccretion
Copy link
Contributor Author

Stress failures:

  1. TypeGeneratorTests400-499: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-78131-merge-7d3e4ae48c114120a0/TypeGeneratorTests400-499/1/console.0f672ed5.log?helixlogtype=result - looks to have passed actually?
12:46:58.844 Passed test: Loader/classloader/TypeGeneratorTests/TypeGeneratorTest499/Generated499/Generated499.dll
Expected: 100
Actual: 100
END EXECUTION - PASSED
+ export _commandExitCode=0
+ /usr/bin/python3 /datadisks/disk1/work/AF3D0989/p/reporter/run.py https://dev.azure.com/dnceng-public/ public 2032048 eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsIng1dCI6Im9PdmN6NU1fN3AtSGpJS2xGWHo5M3VfVjBabyJ9.eyJuYW1laWQiOiJjNzczZjJjMi01MTIwLTQyMDctYWZlMi1hZmFmMzVhOGJjMGEiLCJzY3AiOiJhcHBfdG9rZW4iLCJhdWkiOiJmMjgxYjY5ZS0xYjAzLTRiOWEtYWEyMy0xYzA4YWFhMzRjNTMiLCJzaWQiOiIzNTc3ODYxMS0yNjgxLTRhNTQtYTBkOC1lYWY2MjBhNzE2NTQiLCJCdWlsZElkIjoiY2JiMTgyNjEtYzQ4Zi00YWJiLTg2NTEtOGNkY2I1NDc0NjQ5Ozk1OTg0IiwicHBpZCI6InZzdGZzOi8vL0J1aWxkL0J1aWxkLzk1OTg0Iiwib3JjaGlkIjoiMTEzYzBmZmUtNjg4ZS00OWYxLWIwMDEtZTNhYTM0ODJkZTA1LmJ1aWxkLnJ1bl90ZXN0X3AxX19saW51eF94NjRfY2hlY2tlZC5fX2RlZmF1bHQiLCJyZXBvSWRzIjoiIiwiaXNzIjoiYXBwLnZzdG9rZW4udmlzdWFsc3R1ZGlvLmNvbSIsImF1ZCI6ImFwcC52c3Rva2VuLnZpc3VhbHN0dWRpby5jb218dnNvOjZmY2M5MmU1LTczYTctNGY4OC04ZDEzLWQ5MDQ1YjQ1ZmIyNyIsIm5iZiI6MTY2OTcyMjIxMSwiZXhwIjoxNjY5NzQ2ODExfQ.Sll8uONVwy9ZF9KTir7KbsJEDJ1UOe-vCIk4fL6oQfD52TnPQk082mPT1YKXwTrUuE0xSxNS7ehaYTSH5CGUN-RVDJDkgiJauEH6CLZvnNQrKJySShBowW7azSX7X_XYcpOT8-XafcEKFr_x1r-VUTgbl_GviHcFCTCv-KnHHOqIsj1Ma8DJRUzX3Lax12F2coGlh-ogqt8vm566VBsgTK6lUmJJuMXL3FceWGvVJUnpjdLYdtYcMIvU3s8o2_CE5zIWeVNH7GeqDEAW4afcydK9bQTmDxfffsWiT3zlhp0BO3BGlEZPrg5by-JVspcr30yakIn5YbrYLWmns4vmNg
Killed
['TypeGeneratorTests400-499' END OF WORK ITEM LOG: Command timed out, and was killed]
  1. JIT\\Regression\\JitBlue\\Runtime_63354\\Runtime_63354\\Runtime_63354.cmd, x86: Test failure JIT\\Regression\\JitBlue\\Runtime_63354\\Runtime_63354\\Runtime_63354.cmd #78898.

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

Looks great to me, very nice to no longer have to think about these cases after local morph.
Are you going to optimize other uses (e.g. the liveness change we discussed) in a follow-up?

@jakobbotsch jakobbotsch merged commit be7030f into dotnet:main Nov 30, 2022
@SingleAccretion
Copy link
Contributor Author

Are you going to optimize other uses (e.g. the liveness change we discussed) in a follow-up?

Yep.

@SingleAccretion SingleAccretion deleted the LclMorph-StructUse-Upstream branch November 30, 2022 13:40
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2022
@JulieLeeMSFT
Copy link
Member

JulieLeeMSFT commented Aug 27, 2024

Hi @SingleAccretion, we are trying to clean up TODO comments. You touched this part of the code in this PR,

runtime/src/coreclr/jit/morph.cpp

Lines 11154 to 11167 in 7ff684a

// TODO: support `genReturnBB != nullptr`, it requires #11413 to avoid `Incompatible types for
// gtNewTempStore`.
if (canFold && (genReturnBB == nullptr))
{
// Fold even if types do not match, lowering will handle it. This allows the local
// to remain DNER-free and be enregistered.
assert(lclFld->GetLclOffs() == 0);
lclFld->ChangeType(varDsc->TypeGet());
lclFld->SetOper(GT_LCL_VAR);
}
else if (!varDsc->lvDoNotEnregister)
{
lvaSetVarDoNotEnregister(lclNum DEBUGARG(DoNotEnregisterReason::BlockOpRet));
}
. Our team members do not have much context on it, so I wanted to ask you if you know what to do to support genReturnBB != nullptr.
#11413 is now supported.

        // TODO: support `genReturnBB != nullptr`, it requires #11413 to avoid `Incompatible types for
        // gtNewTempStore`.
        if (canFold && (genReturnBB == nullptr))
        {
            // Fold even if types do not match, lowering will handle it. This allows the local
            // to remain DNER-free and be enregistered.
            assert(lclFld->GetLclOffs() == 0);
            lclFld->ChangeType(varDsc->TypeGet());
            lclFld->SetOper(GT_LCL_VAR);
        }
        else if (!varDsc->lvDoNotEnregister)
        {
            lvaSetVarDoNotEnregister(lclNum DEBUGARG(DoNotEnregisterReason::BlockOpRet));
        }

@JulieLeeMSFT
Copy link
Member

BTW, the bitconverter instructions from #11413 does not hit this code path.

@dotnet dotnet unlocked this conversation Aug 27, 2024
@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Aug 27, 2024

I wanted to ask you if you know what to do to support genReturnBB != nullptr

@JulieLeeMSFT it requires removing the property that allows "ABI-compatible" types under a GT_RETURN node in HIR. What the comment is talking about is that if you have something like:

struct Integer { int Value; }

Integer GetMyInt() {
  if (...)
     return new Integer(1); // A primitive - we can retype that
  else
     return GetAnotherInt(); // A call that return a primitive struct - we don't/can't retype that.
}

If this could be transformed by morph into something like:

if (...)
  GT_RETURN<struct>(CNS_INT<int>(1))
else
  GT_RETURN<struct>(CALL<struct>())

The common return local path would get mismatched types:

if (...)
  getReturnLcl<struct> = CNS_INT<int>(1); // Type mismatch!
else
  getReturnLcl<struct> = CALL<struct>();

GT_RETURN<struct>(getReturnLcl);

It is why this optimization is only done when we don't have a common return local - genReturnBB == nullptr.

@JulieLeeMSFT
Copy link
Member

@JulieLeeMSFT it requires removing the property that allows "ABI-compatible" types under a GT_RETURN node in HIR. What the comment is talking about is that if you have something like:

Thanks @SingleAccretion for a quick response. I will take a look.

@jakobbotsch
Copy link
Member

@JulieLeeMSFT This won't be a simple fix. It overlaps with the work I need to do to enable better ABI treatment with physical promotion, and hopefully I can get to that by .NET 10.
Basically, we can consider this TODO to be tracked by #86665. Feel free to remove the "TODO" part of the comment from the code base now that #86665 calls this scenario out explicitly.

@JulieLeeMSFT
Copy link
Member

Sounds good. I will do that.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 28, 2024
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.

4 participants