Ensure that GenTreeIntrinsic doesn't incorrectly overwrite the gtEntryPoint

[tannergooding]

This resolves #102839 and resolves #102840

The general issue is that some intrinsic nodes no-op and thus return one of the input parameters. In the case this input was GT_INTRINSIC (or as of the recent PR #102702, GT_HWINTRINSIC), then we would end up incorrectly overwriting the gtEntryPoint with the information from the intrinsic that was a no-op.

A simple example is Unsafe.BitCast<double, long>(Math.Sin(x)) where the flow is:

• impIntrinsic for Math.Sin(x) returns a GT_INTRINSIC and sets the gtEntryPoint to Math.Sin
• impIntrinsic for Unsafe.BitCast no-ops and returns the GT_INTRINSIC for NI_Math_Sin and then overwrites gtEntryPoint with Unsafe.BitCast

This could occur for any of the intrinsics with no-op, including things like Unsafe.As, Unsafe.Add when the offset was 0, etc.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[tannergooding]

It's worth noting this is a longstanding but hidden issue, it could be triggered going back several releases (I expect even on .NET Framework using the no-op intrinsic support we had for Vector<T>) given the right constant folding scenarios/intrinsic APIs being called.

So it may be worth considering a backport to .NET 8

[jakobbotsch]

Can it be fixed similarly to #96557? I.e. avoid running the logic in impImportCall unless it's actually the GenTreeCall node that was created by impImportCall.

[jakobbotsch]

Overall it doesn't seem like this should require changes to any GenTree. Rather we should refactor the import path so that this handling just falls out and is naturally skipped when the node isn't freshly created. For example, would it be possible to move the responsibility of handling GT_INTRINSIC into impIntrinsic so that it can naturally skip it when it doesn't make sense?

[tannergooding]

Overall it doesn't seem like this should require changes to any GenTree. Rather we should refactor the import path so that this handling just falls out and is naturally skipped when the node isn't freshly created. For example, would it be possible to move the responsibility of handling GT_INTRINSIC into impIntrinsic so that it can naturally skip it when it doesn't make sense?

That requires passing up a lot of logic in a lot of different places and I think is an overall riskier change.

That is, we would need to pass callInfo->codePointerLookup.constLookup down all the way through impIntrinsic, the various other call paths like impSRCSUnsafeIntrinsic, impPrimitiveNamedIntrinsic, impHWIntrinsic, impSimdAsHWIntrinsic, refactor the logic to not return directly from impIntrinsic but rather to go to a consistent handler before the return that does the SetEntryPoint handling; and/or we would need to track some bool isNewNode parameter through and ensure that any path which creates a new node (or inversely which doesn't create a new node) sets the state correctly.

On the other hande, this PR is relatively simple and just requires us tracking if gtEntryPoint has been set yet, which happens in precisely one location today. The PR is a little bit larger than it really needs to be because I made sure all the field accesses were going through a helper so that asserts could be added where appropriate and avoid any future issues from other refactorings.

[jakobbotsch]

The handling around gtMethodHandle seems much cleaner and more maintainable in the long run, as the issue that occurred here clearly demonstrates. For that field we ensure that it is always fully initialized when a GenTreeIntrinsic is created. Why can't we follow the same pattern for the entry point? I do not see how it would be risky to either add an extra parameter, or change the relevant method parameters to some MethodWithEntryPoint type.

[tannergooding]

The handling around gtMethodHandle seems much cleaner and more maintainable in the long run, as the issue that occurred here clearly demonstrates. For that field we ensure that it is always fully initialized when a GenTreeIntrinsic is created. Why can't we follow the same pattern for the entry point? I do not see how it would be risky to either add an extra parameter, or change the relevant method parameters to some MethodWithEntryPoint type.

The issue is in all the places we have to pass down callInfo->codePointerLookup.constLookup, which is a fairly large struct and only applicable to R2R code. Which means we have many methods in already complex code which now have a new #if defined(FEATURE_READYTORUN) exclusive parameter and which need to be updated to flow things down. It then also needs that logic to understand that it should only set it if comp->opts.ReadyToRun is true and if compDonotInline() is not true.

So rather than centralizing the logic and keeping things done in 1 obvious place, we have dozens of places that are having to replicate some the necessary checks, become less readable due to ifdefs, or other handling. It is overall a much less clean change than the (ignoring the places that were updated to use GetIntrinsicName() and the empty newlines) around 30 lines that this PR actually represents in terms of the fix. The number of additional places also has the potential to impact throughput due to additional checks and handling that's required throughout.

[jakobbotsch]

The issue is in all the places we have to pass down callInfo->codePointerLookup.constLookup, which is a fairly large struct and only applicable to R2R code. Which means we have many methods in already complex code which now have a new #if defined(FEATURE_READYTORUN) exclusive parameter and which need to be updated to flow things down. It then also needs that logic to understand that it should only set it if comp->opts.ReadyToRun is true and if compDonotInline() is not true.

Then create a new type and pass a pointer to it. The field of this type can have the ifdef.

So rather than centralizing the logic and keeping things done in 1 obvious place, we have dozens of places that are having to replicate some the necessary checks, become less readable due to ifdefs, or other handling. It is overall a much less clean change than the (ignoring the places that were updated to use GetIntrinsicName() and the empty newlines) around 30 lines that this PR actually represents in terms of the fix. The number of additional places also has the potential to impact throughput due to additional checks and handling that's required throughout.

I do not see why we should have two separate mechanisms for initializing information in GenTreeIntrinsic. I would much rather than we maintain the invariant that GenTreeIntrinsic's fields are always fully initialized so that it never exists in some indeterminate state that has to be checked via IsEntryPointSet.

[tannergooding]

I do not see why we should have two separate mechanisms for initializing information in GenTreeIntrinsic. I would much rather than we maintain the invariant that GenTreeIntrinsic's fields are always fully initialized so that it never exists in some indeterminate state that has to be checked via IsEntryPointSet.

I don't think you're understanding how complex the requirement of passing this down to every single place we create a new intrinsic node (either GenTreeIntrinsic or GenTreeHWIntrinsic) actually is. We have hundreds of such calls throughout the JIT and they occur in multiple phases, sometimes with the explicit knowledge that the entry point won't be set because we know it will never need to be rewritten to a call.

It is not a trivial task and there is no easy fix for this, even with some type that has an ifdef. The entry point is functionally a lazy field that gets set once and only under some circumstances. It is not guaranteed.

This is really nothing unique or weird either, we have multiple nodes and other types in the JIT that are explicitly mutable, bashed between other types for performance, etc. There is absolutely zero expectation in the codebase that things are always initialized in the constructor.

[jakobbotsch]

sometimes with the explicit knowledge that the entry point won't be set because we know it will never need to be rewritten to a call.

If I'm understanding you right this means that a NOP import path can return an existing GT_INTRINSIC node without an entry point, in which case we will be attaching completely unrelated entry point to it with this code. This does not make the situation better to me.

I'm not saying that the entry point of GenTreeIntrinsic should not be able to contain an empty value. Just that we should ensure that it gets initialized with the right value from the get-go if we can. And we can do that in the call import path, as we do with the method handle.

[tannergooding]

sometimes with the explicit knowledge that the entry point won't be set because we know it will never need to be rewritten to a call.

These are created in later phases, not during import, so there should be no risk there.

Just that we should ensure that it gets initialized with the right value from the get-go if we can

The issue is in flowing this down through all of impIntrinsic, ensuring that it is definitively always set where required, and not having to update the hundreds of other places that create intrinsic nodes to handle that.

GenTreeHWIntrinsic is a little bit simpler because the method handle is never set by the constructor, only by SetMethodHandle and so it could pass down and take the entry point simultaneously. But it still needs to flow through impCall -> impIntrinsic -> impHWIntrinsic -> impHWIntrinsicSpecial, as well as any other future callsites that need it.

GenTreeIntrinsic gets created in a few less places, but still needs that logic as well.

We then need both independent paths to ensure they don't set the information if compDonotInline() is true and ensure that if any of that changes in the future, that both paths will stay in sync with eachother.

[jakobbotsch]

These are created in later phases, not during import, so there should be no risk there.

So there is an extra silent invariant that I'm not allowed to create intrinsic nodes without entry points during import?

We then need both independent paths to ensure they don't set the information if compDonotInline() is true and ensure that if any of that changes in the future, that both paths will stay in sync with eachother.

It's just fine to initialize this field even if we decide to abort inlining due to an intrinsic. There is no issue in doing that.

In the spirit of unblocking the CI and going to bed I'm going to approve this, but I do not think there is a good reason to diverge from how the method handle initialization works, and I think it makes the call importation path even more convoluted than it already is. In the future maybe we can clean up call importation so that it happens in some class that has easy access to all relevant information about the call being imported, so that there would be no need to update a bunch of signatures here.

[tannergooding]

@jakobbotsch can you take another look. I think the necessary information is mostly pushed down to the creation sites like you wanted now.

For GenTreeHWIntrinsic it's relying on the fact that SetMethodHandle is an independent call from the constructor and requires the entryPoint information to be given simultaneously, so its not quite what you wanted but it did push it down to the point the node was created.

There's a new R2RARG define (like DEBUGARG) to help avoid repeated ifdefs elsewhere in the code.

[jakobbotsch]

Nit: we could introduce some gtNewIntrinsic node constructors to avoid some of this boilerplate around initializing an empty lookup. Up to you.

[jakobbotsch]

LGTM! Thanks for doing this.

[tannergooding]

@jakobbotsch, what are your thoughts on backporting this to .NET 8 since it's LTS and can lead to subtle codegen bugs in NAOT/R2R?

[jakobbotsch]

Is this a .NET 8 regression or does the issue exist further back as well? Typically Jeff/tactics want to see a customer report to motivate a backport. I personally wouldn't mind a proactive backport if you think the issue is likely to be hit.

[tannergooding]

Is this a .NET 8 regression or does the issue exist further back as well? Typically Jeff/tactics want to see a customer report to motivate a backport.

Further back as well, I expect we could build a repro for .NET Framework around Vector<T> (several intrinsics exist that generate no-ops) and Math.Sin (a GT_INTRINSIC node that tracks the entry point).

I personally wouldn't mind a proactive backport if you think the issue is likely to be hit.

This is one of those really odd bugs that I expect has been hit, but since it largely only impacts R2R code and the conditions required to hit it are pretty specific, it likely hasn't shown prominently and may have been chalked up to random failures or the like.

-- .NET Framework users are unlikely to be using Vector<T> which is the only case of no-op intrinsics I'm aware of.
-- .NET Core use is more prominent due to the BCL using intrinsics, but we don't often intermix GT_INTRINSIC nodes with cases that could no-op ourselves.

So I don't think it's a priority to backport, but might be something we want to keep in mind if users report strange bugs in the future.