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

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69a3e3b
Fix issue on specialize 'this_type(interface)'
kaizhangNV Dec 23, 2024
604ed2c
formatting
kaizhangNV Dec 31, 2024
46179e4
error fix
kaizhangNV Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ get_git_version(
project(slang VERSION "${SLANG_VERSION_NUMERIC}" LANGUAGES)
set(PROJECT_VERSION "${SLANG_VERSION_FULL}")

# export compile_commands.json for clangd
set(CMAKE_EXPORT_COMPILE_COMMANDS ON)

#
# Global CMake options
#
Expand Down
45 changes: 45 additions & 0 deletions source/slang/slang-ir-specialize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

auto firstArg = inst->getArg(0);
if (!firstParam || !firstArg)
return false;

// If the first parameter is `this_type(interface_type)`, we will try to see if we
// can specialize this type to some concrete type.
//
// This is a corner case that we can't handle during maybeSpecializeGeneric() call. For
// the specialize call like this `specialize(base, this_type(interface_type),
// witness_table)`, this will specialize the function in the witness_table by using the
// 'this_type(interface_type)' as the first argument. However, we cannot handle this
// 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?

{
if (as<IRInterfaceType>(thisType->getConstraintType()))
{
auto argType = firstArg->getDataType();
if (isCompileTimeConstantType(argType))
{
IRBuilder builderStorage(module);
auto builder = &builderStorage;
auto newParam = builder->createParam(argType);

// Replace the first parameter with the new parameter.
auto firstBlock = calleeFunc->getFirstBlock();
firstBlock->insertParamAtHead(newParam);
firstParam->transferDecorationsTo(newParam);
firstParam->replaceUsesWith(newParam);
firstParam->removeAndDeallocate();

// 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.

auto firstParamType = functionType->getParamType(0);
firstParamType->replaceUsesWith(argType);
firstParamType->removeAndDeallocate();
}
}
}

return false;
}

// At this point, we believe we *should* and *can* specialize.
//
Expand Down
Loading