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.

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

Fix crossgen2 of Vector<T> while AbiStable set #46069

Closed
wants to merge 2 commits into from
Closed

Fix crossgen2 of Vector<T> while AbiStable set #46069

wants to merge 2 commits into from

Conversation

t-mustafin
Copy link
Contributor

Methods Vector<T> aot compilation should perform after #40820. I don't exactly know how to count instanceLayout, question #45794, made it with _fallbackAlgorithm.ComputeInstanceLayout().
First two points from #44948 conncted to Vector<T> methods should be fixed by this PR.

calculator_vector_aggressivedel.nettrace.txt

cc @davidwrighton @jkotas @alpencolt

Signed-off-by: Timur [email protected]

@dnfadmin
Copy link

dnfadmin commented Dec 15, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@t-mustafin t-mustafin requested a review from jkotas December 21, 2020 11:31
@jkotas jkotas requested a review from davidwrighton December 21, 2020 16:10
@jkotas
Copy link
Member

jkotas commented Dec 21, 2020

@davidwrighton should review this change. It may need to wait after the holidays.

@alpencolt
Copy link

@davidwrighton had you chance to look into this change?

@t-mustafin
Copy link
Contributor Author

Crossgen2 precompiled methods added by this pull request https://gist.github.com/t-mustafin/0720c0829328925095f29c29eaecff72

@t-mustafin t-mustafin mentioned this pull request Jan 29, 2021
Closed
Base automatically changed from master to main March 1, 2021 09:07
@mangod9
Copy link
Member

mangod9 commented Mar 1, 2021

@davidwrighton could you please review, so this can be merged?

davidwrighton
davidwrighton requested changes Mar 18, 2021
View reviewed changes
Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

We aren't prepared to modify the base libraries in the way you suggest. Instead please handle the situation internally in crossgen2.

@davidwrighton
Copy link
Member

Other than the libraries changes this looks good.

@mangod9
Copy link
Member

mangod9 commented Apr 12, 2021

@t-mustafin could you please update based on @davidwrighton review?

@mangod9
Copy link
Member

mangod9 commented May 10, 2021

Hi @t-mustafin, do you plan to update the PR or should this be closed for now?

@t-mustafin
Copy link
Contributor Author

Hi @t-mustafin, do you plan to update the PR or should this be closed for now?

Hi, I plan to update it in couple of days, after testing new patch.

@t-mustafin t-mustafin requested a review from davidwrighton May 13, 2021 11:24
davidwrighton
davidwrighton requested changes May 14, 2021
View reviewed changes
Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

Actually. this change breaks the arm test pass thoroughly. Please restrict any changes to your scenario only. Possibly by making this happen only if there is a command line switch to enable this or something.

@davidwrighton
Copy link
Member

@t-mustafin This change causes a great many tests to fail as you can see in the reports, and will not be merged until the tests are clean.

@t-mustafin
Copy link
Contributor Author

@t-mustafin This change causes a great many tests to fail as you can see in the reports, and will not be merged until the tests are clean.

That PR was consists of two patches which fixes first two points of issue #44948. Now I can not suggest a good solution for second point. So maybe only first patch will show no tests regression.

davidwrighton
davidwrighton requested changes Jun 23, 2021
View reviewed changes
Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

See comment about abi stability check for the owningDefType

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
@@ -1035,7 +1035,7 @@ private uint getMethodAttribsInternal(MethodDesc method)
DefType owningDefType = method.OwningType as DefType;
if (owningDefType != null && VectorOfTFieldLayoutAlgorithm.IsVectorOfTType(owningDefType))
{
throw new RequiresRuntimeJitException("This function is using SIMD intrinsics, their size is machine specific");
VerifyMethodSignatureIsStable(method.Signature);
Copy link
Member

Choose a reason for hiding this comment

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

This needs to check that the abi of owningDefType is stable in addition to the signature. I think that will fix the failures you are seeing in the X64 tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it don't fixes Debug build problem.

Copy link
Member

Choose a reason for hiding this comment

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

:-/ Well, It looks reasonable now, but I don't know why its failing then. Let us know when you find out why.

@mangod9
Copy link
Member

mangod9 commented Jul 19, 2021

Is this is required for .net 6 we only have a couple of weeks to get this merged.

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@davidwrighton davidwrighton mentioned this pull request Aug 3, 2021
Open
@mangod9
Copy link
Member

mangod9 commented Sep 13, 2021

@t-mustafin could we close this till you can get back to this?

@alpencolt
Copy link

@mangod9 yes close PR, it looks obsolete. We'll open new one when return to task.

@mangod9 mangod9 closed this Sep 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 21, 2021
@t-mustafin t-mustafin deleted the 44948_aot_vector branch August 10, 2023 14:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davidwrighton davidwrighton davidwrighton requested changes

@jkotas jkotas Awaiting requested review from jkotas

Assignees
No one assigned
Labels
area-crossgen2-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@t-mustafin @dnfadmin @jkotas @alpencolt @mangod9 @davidwrighton @terrajobst @Dotnet-GitSync-Bot