Updating the x86 HWIntrinsic importer to not inline when returning gtNewMustThrowException#15772
Conversation
|
There are actually 3 other cases that will likely need to get fixed as well. These are the I'm currently building for x86 locally and testing the fix works before I push. |
|
FYI. @fiigii |
|
All cases should be fixed. |
|
FYI. @AndyAyersMS. Updated to pass down |
|
Applied the formatting patch. |
| GenTree* impSSEIntrinsic(NamedIntrinsic intrinsic, | ||
| CORINFO_METHOD_HANDLE method, | ||
| CORINFO_SIG_INFO* sig, | ||
| bool mustExpand); |
There was a problem hiding this comment.
Why do you pass mustExpand into impXXXIntrinsic, it seems only required by impX86HWIntrinsic.
There was a problem hiding this comment.
It is also used (in this PR) by Crc32, Lzcnt, and Popcnt, since they throw PNSE when varTypeIsLong(callType)
It will also need to be used by instructions like Sse.Shuffle to determine whether we should fallback to a GT_CALL or allow the instruction to be inlined (when op3 is not a constant).
There was a problem hiding this comment.
I think all the exception code can be unified to impX86HWIntrinsic with the table-driven approach. Now, perhaps you can directly extract the scalar intrinsic's exception code to impX86HWIntrinsic. Then I will improve it in the table-driven PR.
There was a problem hiding this comment.
We should probably do that particular refactoring with the table driven PR. The varTypeIsLong check shouldn't be for all instructions even after refactoring, so there isn't a good place to put it in impX86HWIntrinsic beforehand.
| { | ||
| // When we aren't supported we need to return nullptr when attempting to expand inline | ||
| // (gtIsRecursiveCall == false) as the gtNewMustThrowException node can mess with the | ||
| // inliner in unexpected ways. |
There was a problem hiding this comment.
I'll try and clarify.
Basically, this is saying we've hit a failure case and we need to return nullptr if mustExpand is false, since that means we were attempting to expand the intrinsic inline another method. However, when mustExpand is true, it means we are expanding the intrinsic inline a GT_CALL to ourselves, so we can return the exception.
We do this because the inliner expects the intrinsic, not an exception, and will otherwise fail.
There was a problem hiding this comment.
Fixed. Hopefully the new wording is easier to understand.
| impPopStack(); | ||
| } | ||
|
|
||
| return gtNewMustThrowException(CORINFO_HELP_THROW_PLATFORM_NOT_SUPPORTED, JITtype2varType(sig->retType), |
There was a problem hiding this comment.
Perhaps, parametrizing the exception would be better. We will need other exceptions.
There was a problem hiding this comment.
What other exceptions are we going to need (I'll parameterize it, just interested as I wasn't aware of others)?
There was a problem hiding this comment.
Maybe we will need other exceptions for out of range arguments and non-numeric type arguments, but depends on our future implementation.
There was a problem hiding this comment.
Ok.
I had thought we were just going to throw PNSE for those as well, but have a more explicit failure makes sense as well.
…NewMustThrowException
|
Logged https://github.com/dotnet/coreclr/issues/15778 for the CROSS check issue. |
|
@jkotas, am I good to merge this, despite the CROSS check issue, or should I wait for that to be resolved first? |
I think so. |
|
@dotnet-bot test cross please |
|
@dotnet-bot help |
5 participants
Fixes a case where the inliner asserts when optimizations are enabled (see #15771 (comment)).