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

Join does not always respect the order of provided parameters (#3511) #3513

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

willyborn
Copy link
Contributor

@willyborn willyborn commented Oct 15, 2023

Arrays were be joined in the order the JIT engine generated the arrays iso the order of the parameters.

Description

  1. A wrong assumption was made that the JIT engine would generate the arrays in the same order as provided by the parameters.
    This assumption is violated when one of the generated arrays is an intermediate result of a previous specified JIT generated array.
    Example of error condition:
af::array A{af::constant(1.,10)};
af::array R{af::join(0,-A,A)};  --> generated af::join(0,A,-A) because A is an intermediate result of -A.

In this fix, the order of the parameters is imposed, independent from the order of JIT generation.
PS: This bug appears in OPENCL and CUDA.

  1. The same 'unique' identifier "funcName" was generated for JIT kernels, with outputs as only difference.
    As result, a different kernel could be executed than intended, dependent on the order of the JIT kernel generation.

Additional information about the PR answering following questions:

  • bug fix
  • can be back ported
  • extra test is added covering a large set of possible combinations of similar outputs (567 combinations)

Fixes: #3511

Changes to Users

None

Checklist

  • Rebased on latest master
  • Code compiles
  • Tests pass
  • Functions added to unified API
  • Functions documented

@willyborn
Copy link
Contributor Author

Do not merge yet.
I discovered a new combination giving unexpected results.
The testing case will be expended to include more logical combinations (JIT nodes, Block nodes, repeating, dependencies, ...)
You will get an updated PR in the coming days.

@willyborn
Copy link
Contributor Author

Code is updated and join tests are expanded with the generation of multiple join combinations.

For some joins, the kernel of a previous join could be used since both generated the same 'unique' funcName (while having a different kernel) resulting in the reuse of the previously cached kernel iso generating the new one.
The chance of happening this is very rare, explaining why I added a systematic combination generator to the tests.

@willyborn willyborn changed the title Join does not always respect the order of provided parameters (#3511) Join does not always respect the order of provided parameters (#3511 & #3532) Feb 13, 2024
@willyborn willyborn changed the title Join does not always respect the order of provided parameters (#3511 & #3532) Join does not always respect the order of provided parameters (#3511) Feb 20, 2024
@umar456
Copy link
Member

umar456 commented Mar 7, 2024

Updated the getFuncName parameters in oneapi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] found on AF 3.8.3 and 3.9.0 for OPENCL, possibly with af::join
2 participants