Keep instrumentings some intrinsics #85130
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsAddress comment in #84517 (comment) Some [Intrinsic] methods aren't expanded and we should not ignore them for instrumentation @AndyAyersMS PTAL
|
src/coreclr/jit/fgprofile.cpp
Outdated
| return PhaseStatus::MODIFIED_NOTHING; | ||
| bool shouldBeInstrumented = false; | ||
|
|
||
| // Some intrinsics should still be instrumented. |
There was a problem hiding this comment.
Maybe say a bit more about why these particular intrinsics are the ones that should still be instrumented? In particular, if somebody adds a new intrinsic, how will they know if it should go on this list or not?
There was a problem hiding this comment.
Can we filter it primarily based on whether or not its recursive?
In general things that are recursive are considered "must expand" and never have a meaningful implementation, while things that are non-recursive are only "might expand" and it can depend on factors such as optimization level, what the inputs are, what the hardware supports, etc.
We could then still have this switch to handle the other cases and we could default to instrumenting with some selective logic for things we still don't want to instrument (say Math.Floor when Sse4.1 is supported).
-- This would also be a motivation to finish up the S.R.CS.Unsafe work for example, as we were planning on making those recursive once that was completed.
There was a problem hiding this comment.
@AndyAyersMS added comments. We might want to compose a single .h table where we'll list all these properties for each intrinsics (e.g. display name, instrumentability, etc)
There was a problem hiding this comment.
Can we filter it primarily based on whether or not its recursive?
Not really unless you go and implement them all in Mono too (and interp) 🙂
The existing Tier0 must-expand won't hit this path because Tier0 will inline them so they won't be instrumented anyway
ghost
locked as resolved and limited conversation to collaborators
Address comment in #84517 (comment)
Some [Intrinsic] methods aren't expanded and we should not ignore them for instrumentation
@AndyAyersMS PTAL