Change the ReciprocalEstimate and ReciprocalSqrtEstimate APIs to be mustExpand on RyuJIT

[tannergooding]

This resolves #101731

[tannergooding]

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]

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]

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

[jkotas]

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

[tannergooding]

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

[tannergooding]

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]

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

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

[jakobbotsch]

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

[jakobbotsch]

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

[tannergooding]

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.

[tannergooding]

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.

[tannergooding]

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.

[tannergooding]

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

[jakobbotsch]

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.

[tannergooding]

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

[jakobbotsch]

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.

[jkotas]

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.

[jakobbotsch]

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.