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

Fix issue on specialize 'this_type(interface)' #5970

Closed
wants to merge 3 commits into from

Conversation

kaizhangNV
Copy link
Contributor

@kaizhangNV kaizhangNV commented Dec 31, 2024

close issue #5900

@kaizhangNV kaizhangNV added the pr: non-breaking PRs without breaking changes label Dec 31, 2024
@kaizhangNV kaizhangNV requested a review from a team as a code owner December 31, 2024 05:17
Copy link
Collaborator

@csyonghe csyonghe left a comment

Choose a reason for hiding this comment

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

I want to get better understanding why IRThisType can appear as the parameter type of an actual function, this seems wrong to me.

// case in maybeSpecializeGeneric() because we the concrete type of the
// 'this_type(interface_type)' is not known at that time. So we have to handle this case
// here.
if (auto thisType = as<IRThisType>(firstParam->getDataType()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just check for ThisType in the loop above at line 1316?


// Replace the first parameter of the function type with the
// concrete type.
auto functionType = calleeFunc->getDataType();
Copy link
Collaborator

Choose a reason for hiding this comment

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

firstParamType->replaceUsesWith() is dangerous/incorrect, because there may still be other uses of the type (e.g. outside the function we are processing), and they shouldn't be replaced.

If the intention here is just to create a new IRFuncType and set it to calleeFunc, you can just call fixupFuncType before returning.

@@ -1328,7 +1328,52 @@ struct SpecializationContext
// If we never found a parameter or return type worth specializing, we should bail out.
//
if (!returnTypeNeedSpecialization && !argumentNeedSpecialization)
{
auto firstParam = calleeFunc->getFirstParam();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not understanding the underlying issue we are trying to fix here.

How can a param type of the callee end up being an ThisType? IIRC ThisType is only used when declaring an interface requirement, and should never appear in any actual methods. I am very confused why the callee here can have its first param type being an IRThisType. They should only just be IRInterfaceType and be handled with the existing code.

Copy link
Contributor Author

@kaizhangNV kaizhangNV Dec 31, 2024

Choose a reason for hiding this comment

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

call specialize(base, this_type(interface_type), witness_table) (xxx)

for above IR, maybeSpecializeGeneric() will evaluate specialize, and this will generate a specialized function with this_type(interface_type) as the input parameter.

Let's say the function is named func, then this IR will be

call func(xxx), and the function will be

void func(this_type(interface_type))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on what you said, you mean

call specialize(base, this_type(interface_type), witness_table) (xxx)

this_type(interface_type) should never appear in specialize call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know about that.
But if this is the case, then the bug is in more previous stage.

@csyonghe
Copy link
Collaborator

The PR should also include a regression test for the issue we are fixing.

@kaizhangNV
Copy link
Contributor Author

The PR should also include a regression test for the issue we are fixing.

Yea, I will add.

@csyonghe
Copy link
Collaborator

I did some investigation here, and it appears that the IR for bugTest is wrong after lower-to-ir.

It is something like:

call(specialize(extension::load, ThisType), ...)

The specialize(extension::load, ThisType) is an invalid inst. We should never use ThisType to specialize anything.

This usually occurs when the front-end produces wrong DeclRef to extension::load after type checking. It currently is something like load<ThisType>, which makes no sense. It really should be invoking load<ExtractExistentialType(bugTest::t)> instead.

@csyonghe
Copy link
Collaborator

I drafted an attempt to fix this issue in the front-end here: #5971

It may still not be 100% correct, but this is the direction we should be exploring.

@kaizhangNV
Copy link
Contributor Author

It makes sense to me that we should fix the front-end if the ThisType should not appear in specialize().
Close this PR.

@kaizhangNV kaizhangNV closed this Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants