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

Change the ReciprocalEstimate and ReciprocalSqrtEstimate APIs to be mustExpand on RyuJIT #102098

Merged
merged 17 commits into from
May 13, 2024

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented May 10, 2024

This resolves #101731

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 10, 2024
@tannergooding tannergooding added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 10, 2024
@tannergooding tannergooding marked this pull request as ready for review May 11, 2024 02:43
case NI_Vector128_ConvertToInt32Native:
{
if (opts.IsReadyToRun())
Copy link
Member Author

Choose a reason for hiding this comment

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

I considered making this a flag in the hwintrinsic list table, to allow this to be more easily centralized for any future APIs that fit in this bucket, but opted not to since we don't have many spare bits for the that flags enum right now and this helps keep the cost lower for the main intrinsic path.

@jkotas
Copy link
Member

jkotas commented May 11, 2024

src/tests/ilverify/TestDataLoader.cs(272,45): error CS0535: 'TestDataLoader.TestResolver' does not implement interface member

I am fixing this outer loop build break in #102115

@tannergooding
Copy link
Member Author

CC. @jkotas this should be ready now. Only spmi is left and they are rerunning due to failing to download the mch file

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM! It can use a second sign-off from codegen team.

@tannergooding
Copy link
Member Author

CC. @dotnet/jit-contrib for secondary review. This puts CI into a clean state again

@tannergooding
Copy link
Member Author

tannergooding commented May 13, 2024

The remaining Arm64 failure is different from the issue that #101731 is tracking (and not unique to this PR).

I'm looking into it, but it would probably be good to get this one merged

@tannergooding
Copy link
Member Author

Ping @dotnet/jit-contrib for review, this resolves the main CI issue.

#102162 fixes the remaining pre-existing issue, but is libraries side only

{
if (mustExpand)
{
implLimitation();
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly there is no JIT side implementation limitation here. This is an EE side policy that ideally ought to be applied there (for example by some bool onNonDeterministicIntrinsic(bool mustExpand) JIT-EE function that returns true if the JIT should expand it).

This approach will show up in the crossgen2 log with a misleading "JIT implementation limitation" error. Does that only happen inside our own definitions of these intrinsics? If so I guess I can live with it (and in any case if changing this we can do it in a follow up to unblock CI asap).

Copy link
Member

Choose a reason for hiding this comment

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

Does this message show up to end users if they use composite mode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does that only happen inside our own definitions of these intrinsics

Yes, only for the recursive expansion. The regular expansion is handled gracefully by simply not doing it, so it persists as a regular CALL instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this message show up to end users if they use composite mode?

Not sure. Happy to test, just not familiar with the best way to do that however.

Copy link
Member Author

Choose a reason for hiding this comment

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

moving that expansion policy check to the EE side

Could you elaborate a bit on how you would expect this to work?

It's unclear to me how the JIT would use this to fail compilation for the method. We can certainly have the VM say that the JIT is compiling for an unknown machine (which differs from JIT which is current machine and NAOT which is a specific machine), but the JIT would still only be able to call implLimitation() here as a result of that JIT/EE call.

The VM could avoid trying to compile such a method entirely, but that would entail it doing the check for whether the call is recursive and that's not always trivial to do given the various if/else patterns and constant folding that can occur. The JIT is really the best thing equipped to detect the actual recursion.

Copy link
Member Author

Choose a reason for hiding this comment

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

You would throw RequiresRuntimeJitException with an appropriate message from crossgen2 in the mustExpand case, and otherwise return whether or not the non-deterministic intrinsic should be expanded by the JIT.

So it's less bool onNonDeterministicIntrinsic(bool mustExpand) and more void notifyNonDeterministicIntrinsicUsage(NamedIntrinsic intrinsic, bool mustExpand) or something to that effect, which then allows the VM to decide to throw out the compilation result (such as by throwing or some other mechanism).

Copy link
Member

Choose a reason for hiding this comment

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

This message will show up in verbose mode only. It is similar to what you get for non-implemented explicit tailcalls in R2R.

Explicit tailcalls in crossgen2 do not result in "JIT implementation limitation". I have personally used these messages to investigate our cases where we fail R2R compilations, and if I saw "JIT implementation limitation" I would assume something we should be fixing on the JIT side as implementation limitations within the JIT should not be hittable in practice.

There is not much policy to apply. The non-deterministic expansion is a problem for any form of R2R, and fine everywhere else.

Completely failing a compilation is a pretty large decision. Having to abuse implLimitation() to do so is an indication to me that it should be done on the EE side.

So it's less bool onNonDeterministicIntrinsic(bool mustExpand) and more void notifyNonDeterministicIntrinsicUsage(NamedIntrinsic intrinsic, bool mustExpand) or something to that effect, which then allows the VM to decide to throw out the compilation result (such as by throwing or some other mechanism).

That would be another way to do it, I was just wanting to move all of the policy to the EE side in a single JIT-EE call.

Copy link
Member Author

@tannergooding tannergooding May 13, 2024

Choose a reason for hiding this comment

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

Happy to do the work here if it's desired.

I do, in general, like the premise of a JIT/EE call of the shape bool notifyNonDeterministicOperationUsage(SomeIdentifier identifier, bool mustExpand) const. We have many places in the JIT today that are querying opts.IsReadyToRun() and potentially not accounting for NAOT (such as in valuenum), many of these places are specifically just trying to consider the fact that some edge case handling is non-deterministic and figure out whether something like constant folding is "ok". So having something that allows us to basically query if the VM supports seems beneficial. Longer term, it would also allow playing into multiple compilations if that was ever desired (say compiling once for legacy, once for VEX, once for EVEX, etc).

Such a shape matches the general look of bool notifyInstructionSetUsage(CORINFO_InstructionSet isa, bool supported) const and plays into the same consideration of being able to simply say "I'm trying to do this, is it okay" and with the expectation that mustExpand == true can't return false

Copy link
Member

Choose a reason for hiding this comment

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

I certainly am not going to lose sleep over not changing this right at this moment. If you think this will be more broadly useful elsewhere then I don't mind leaving this as a clean up to happen as part of that work when we get to it.

Copy link
Member

Choose a reason for hiding this comment

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

notifyNonDeterministicOperationUsage(SomeIdentifier identifier, bool mustExpand)

What would the EE side of this callback look like? I think that the EE side will either implement this as unconditional no-op (for non-resilient codegen) or as unconditional exception throw (for resilient codegen). The JIT is told whether to generate resilient or non-resilient code via compilation JIT flags. I do not see the point in asking EE about whether you really meant to generate resilient code.

We have many places in the JIT today that are querying opts.IsReadyToRun() and potentially not accounting for NAOT (such as in valuenum),

I have glanced over valuenum and I do not see any problems there. There is a history behind why NAOT is treated as R2R. It does not necessarily match how some devs think about it these days. I would be fine with using less ambiguous names here.

misleading "JIT implementation limitation" error

I see this as JIT implementation limitation. We can implement this properly in the JIT and get rid of BlockNonDeterministicIntrinsics if we want. It would require the JIT to figure out the best hardware level to target (same as what we do in other places that use hw intrinsics opportunistically), and let the EE know that the code took a dependency on the exact hardware spec via notifyInstructionSetUsage. We may need to expand the notifyInstructionSetUsage to make it work well and future proof.

Short of that, if we do not like the "JIT implementation limitation" message being show to the user in the verbose log, we can introduce JIT/EE interface implementationLimination(char* message) method that shows an implementation limitation error with a custom message.

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

The JIT side LGTM, but I'd still suggest moving that expansion policy check to the EE side so it can give an appropriate error message if users are potentially going to see these messages in composite mode.

@tannergooding tannergooding merged commit 3bd4171 into dotnet:main May 13, 2024
173 of 175 checks passed
@tannergooding tannergooding deleted the fix-101731 branch May 13, 2024 21:01
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
…ustExpand on RyuJIT (dotnet#102098)

* Change the ReciprocalEstimate and ReciprocalSqrtEstimate APIs to be mustExpand on RyuJIT

* Apply formatting patch

* Fix the RV64 and LA64 builds

* Mark the ReciprocalEstimate and ReciprocalSqrtEstimate methods as AggressiveOptimization to bypass R2R

* Mark other usages of ReciprocalEstimate and ReciprocalSqrtEstimate in Corelib with AggressiveOptimization

* Mark several non-deterministic APIs as BypassReadyToRun and skip intrinsic expansion in R2R

* Cleanup based on PR recommendations to rely on the runtime rather than attributation of non-deterministic intrinsics

* Adding a regression test ensuring direct and indirect invocation of non-deterministic intrinsic APIs returns the same result

* Add a note about non-deterministic intrinsic expansion to the botr

* Apply formatting patch

* Ensure vector tests are correctly validating against the scalar implementation

* Fix the JIT/SIMD/VectorConvert test and workaround a 32-bit test issue

* Skip a test on Mono due to a known/tracked issue

* Ensure that lowering on Arm64 doesn't make an assumption about cast shapes

* Ensure the tier0opts local is used

* Ensure impEstimateIntrinsic bails out for APIs that need to be implemented as user calls
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
3 participants