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

[DNM] Attempt to make FEP selection at compile time #13425

Open
wants to merge 3 commits into
base: emitMSIL
Choose a base branch
from

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Oct 25, 2022

Purpose

This attempts to make FEP selection at compile time. Currently, given multiple FEPs belonging to a function group, we are simply choosing the first one, which can be the wrong one. In our current tests, there are some cases where the first one is the wrong FEP that is chosen but at runtime, it sorts itself out in the call to ComputeFeps where the correct one is resolved before the final call. However, I believe there can be cases (not encountered in tests so far) where the first FEP chosen at compile time is completely erroneous (static <=> instance) or has the wrong number of parameters. I will add a TODO to try adding a failing test for this case. Further, in the case of a non-replicated call, it is absolutely essential that we pick the correct FEP at compile time as we cannot fall back on runtime FEP selection in this case.

This PR attempts to introduce a compile-time FEP selection function but it turns out to be inadequate as it doesn't consider all cases, such as arguments and parameters differing in ranks, and ranking candidate FEPs in order of how close they match with the arguments, it just chooses the first matching one (based on argument number and checking if args can be coerced into param types). I then attempted to use at compile-time the same FEP selection logic in SelectFinalFep that is called at run time. Note that runtime FEP selection cannot be replaced entirely as it requires runtime values for the most accurate results obviously.

TODO:

  • Add failing test for the wrong FEP selected at compile time that cannot be resolved to the correct one at runtime either.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@aparajit-pratap aparajit-pratap added DNM Do not merge. For PRs. WIP labels Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DNM Do not merge. For PRs. WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant