Skip to content
This repository was archived by the owner on Mar 28, 2023. It is now read-only.

InvokeSimd: Added named barrier and SLM access tests #1621

Merged
merged 5 commits into from
Mar 10, 2023
Merged

InvokeSimd: Added named barrier and SLM access tests #1621

merged 5 commits into from
Mar 10, 2023

Conversation

fveselov
Copy link

No description provided.

@fveselov fveselov requested a review from a team as a code owner February 27, 2023 17:24
@fveselov fveselov requested a review from sarnex March 1, 2023 14:02
Copy link

@v-klochkov v-klochkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have few non-blocking comments and one more general concern.
If I remember it correctly, slm_init() function will not work in such context and may require passing local accessor instead and re-working the test.

Sorry, I don't remember all the details, it will take some time.

@fveselov
Copy link
Author

fveselov commented Mar 6, 2023

I have few non-blocking comments and one more general concern. If I remember it correctly, slm_init() function will not work in such context and may require passing local accessor instead and re-working the test.

Sorry, I don't remember all the details, it will take some time.

If I understand correctly, the interface for allocating SLM outside of invoke_simd isn't yet settled.

At the moment I added following line to suppress these tests:
// REQUIRES: TEMPORARY_DISABLED

@v-klochkov, Should we wait till the Jira issue resolved or PR can be merged now?

@v-klochkov
Copy link

v-klochkov commented Mar 7, 2023

I have few non-blocking comments and one more general concern. If I remember it correctly, slm_init() function will not work in such context and may require passing local accessor instead and re-working the test.
Sorry, I don't remember all the details, it will take some time.

If I understand correctly, the interface for allocating SLM outside of invoke_simd isn't yet settled.

At the moment I added following line to suppress these tests: // REQUIRES: TEMPORARY_DISABLED

@v-klochkov, Should we wait till the Jira issue resolved or PR can be merged now?

It probably, should not wait JIRA resolution.

Does the test pass with UseSLM = false? If Yes, then it makes sense to to enable the test for that path and add something like:
// TODO: Enable the sub-tests with UseSLM=true after the issue with SLM initialization is fixed.

Copy link

@v-klochkov v-klochkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the tests are a bit raw - they may use some functionality like SLM allocation or barriers not quite correctly, but that is caused by uncertainty in how that needs to be implemented in ESIMDS. Tests are:
a) close to any user would try to do,
b) good starting point to start developing/fixing ESIMD part,
c) will be updated together with ESIMD changes implementing support in a more proper way.

@v-klochkov v-klochkov merged commit ab3efc1 into intel:intel Mar 10, 2023
@fveselov fveselov deleted the invoke_simd_nbarriers branch March 10, 2023 15:10
myler pushed a commit to myler/llvm-test-suite that referenced this pull request Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants