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 clsHnd in impImportCall #93176

Closed
wants to merge 1 commit into from
+11 −9
Closed

Fix clsHnd in impImportCall #93176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2810,7 +2810,6 @@ class Compiler
GenTreeHWIntrinsic* gtNewScalarHWIntrinsicNode(
var_types type, GenTree* op1, GenTree* op2, GenTree* op3, NamedIntrinsic hwIntrinsicID);
CorInfoType getBaseJitTypeFromArgIfNeeded(NamedIntrinsic intrinsic,
CORINFO_CLASS_HANDLE clsHnd,
CORINFO_SIG_INFO* sig,
CorInfoType simdBaseJitType);

Expand Down
10 changes: 4 additions & 6 deletions src/coreclr/jit/hwintrinsic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,18 +359,16 @@ const TernaryLogicInfo& TernaryLogicInfo::lookup(uint8_t control)
//
// Arguments:
// intrinsic -- id of the intrinsic function.
// clsHnd -- class handle containing the intrinsic function.
// method -- method handle of the intrinsic function.
// sig -- signature of the intrinsic call.
// simdBaseJitType -- Predetermined simdBaseJitType, could be CORINFO_TYPE_UNDEF
//
// Return Value:
// The basetype of intrinsic of it can be fetched from 1st or 2nd argument, else return baseType unmodified.
//
CorInfoType Compiler::getBaseJitTypeFromArgIfNeeded(NamedIntrinsic intrinsic,
CORINFO_CLASS_HANDLE clsHnd,
CORINFO_SIG_INFO* sig,
CorInfoType simdBaseJitType)
CorInfoType Compiler::getBaseJitTypeFromArgIfNeeded(NamedIntrinsic intrinsic,
CORINFO_SIG_INFO* sig,
CorInfoType simdBaseJitType)
{
if (HWIntrinsicInfo::BaseTypeFromSecondArg(intrinsic) || HWIntrinsicInfo::BaseTypeFromFirstArg(intrinsic))
{
Expand Down Expand Up @@ -1088,7 +1086,7 @@ GenTree* Compiler::impHWIntrinsic(NamedIntrinsic intrinsic,
}
}

simdBaseJitType = getBaseJitTypeFromArgIfNeeded(intrinsic, clsHnd, sig, simdBaseJitType);
simdBaseJitType = getBaseJitTypeFromArgIfNeeded(intrinsic, sig, simdBaseJitType);

if (simdBaseJitType == CORINFO_TYPE_UNDEF)
{
Expand Down
9 changes: 7 additions & 2 deletions src/coreclr/jit/importercalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,15 @@ var_types Compiler::impImportCall(OPCODE opcode,
}
}

clsHnd = pResolvedToken->hClass;

clsHnd = pResolvedToken->hClass;
clsFlags = callInfo->classFlags;

// if clsHnd is an interface and method is static, try to get the actual implementation class
if ((clsFlags & CORINFO_FLG_INTERFACE) != 0 && (clsFlags & CORINFO_FLG_INTERFACE) != 0)
Copy link
Member

@jkotas jkotas Oct 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There are other situations where getCallInfo can resolve the method to a method on a different class. Would it be better to return the class handle of the resolved method in CORINFO_CALL_INFO?

Copy link
Member

Choose a reason for hiding this comment

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

(Also, the condition has typo - it checks CORINFO_FLG_INTERFACE twice.)

Copy link
Member Author

Choose a reason for hiding this comment

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

@jkotas do you mean getCallInfo should patch input resolveToken->hClass? Because it seems that JIT mostly access hClass directly from resolveToken for methods in importer

Copy link
Member

Choose a reason for hiding this comment

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

I meant that there should be a new CORINFO_CLASS_HANDLE hClass; //target class handle field in CORINFO_CALL_INFO.

Patching CORINFO_RESOLVED_TOKEN would be a hack. CORINFO_RESOLVED_TOKEN is meant to be "in" argument for getCallInfo.

it seems that JIT mostly access hClass directly from resolveToken for methods in importer

I would expect that some places want the hClass of the method referenced by the IL and some places want the hClass of the method that the VM resolved the method to. I would not be surprised if there are subtle bugs today like the one you are trying to fix.

{
clsHnd = info.compCompHnd->getMethodClass(methHnd);
}

#ifdef DEBUG
// If this is a call to JitTestLabel.Mark, do "early inlining", and record the test attribute.

Expand Down