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

[SYCL] Put constant initializer list data in non-generic addr space. #3005

Merged
merged 4 commits into from
Jan 13, 2021

Conversation

kbobrovs
Copy link
Contributor

@kbobrovs kbobrovs commented Jan 9, 2021

Big constant initializer lists (>= 16 elements) were represented as a
global in the generic address space. This is incorrect, as this is an
alias AS, and allocation is not possible in it - compiler will have no
clue where to really put data. Gen vector BE crashed on this pattern.
The fix is to put this data into the same address space as a string
literal.

End-to-end test for ESIMD (Gen vector BE) is underway to llvm-test-suite:
intel/llvm-test-suite#91

Signed-off-by: Konstantin S Bobrovsky [email protected]

Big constant initializer lists (>= 16 elements) were represented as a
global in the generic address space. This is incorrect, as this is an
alias AS, and allocation is not possible in it - compiler will have no
clue where to really put data. Gen vector BE crashed on this pattern.
The fix is to put this data into the same address space as a string
literal.

Signed-off-by: Konstantin S Bobrovsky <[email protected]>
@bader
Copy link
Contributor

bader commented Jan 11, 2021

@kbobrovs, I think #2864 addresses this problem as well. Could you confirm that, please?

If so, I prefer we commit #2864 instead to avoid conflicts between these two patches.
#2864 implements the approach suggested by clang's CodeGen code owner and significantly reduces the source code difference with llorg.

@kbobrovs
Copy link
Contributor Author

kbobrovs commented Jan 12, 2021

@bader, yes, it seems #2864 solves the problem fixed by #3005. Please add a test case from #3005 to #2864 to make sure the scenario we care about works. I will then close #3005.

@bader
Copy link
Contributor

bader commented Jan 12, 2021

@bader, yes, it seems #2864 solves the problem fixed by #3005. Please add a test case from #3005 to #2864 to make sure the scenario we care about works. I will then close #3005.

I've added similar test to #2864 and it doesn't work for me. The constant array used in initializer list is allocated in generic address space. I'm going to fix this case in scope #2864. Feel free to proceed with this PR, if it's an urgent - fixing regressions caused by #2864 will take some time.

@kbobrovs
Copy link
Contributor Author

Feel free to proceed with this PR, if it's an urgent - fixing regressions caused by #2864 will take some time.

It is pretty urgent - affects important users scenarios. Since this fix is just 3 lines, it should not be hard to replace it with #2864 when it is ready, I believe. So yes, I'd prefer to proceed with this one then.

Signed-off-by: Konstantin S Bobrovsky <[email protected]>
Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@kbobrovs
Copy link
Contributor Author

End-to-end test for ESIMD (Gen vector BE) is underway to llvm-test-suite.

intel/llvm-test-suite#91

@kbobrovs
Copy link
Contributor Author

@premanandrao - please review/approve as the code owner

Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

LGTM

@kbobrovs kbobrovs merged commit 0f2cf4d into intel:sycl Jan 13, 2021
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Jan 19, 2021
* sycl: (378 commits)
  [sycl-post-link][NFC] Extracted the code into a subroutine (intel#3042)
  [SYCL][NFC] Remove commented out code (intel#3029)
  [CODEOWNERS] Fix ownership of DPC++ tools tests (intel#3047)
  [SYCL][NFC] Make tests insensitive to dso_local (intel#3037)
  [SYCL] Fix acquiring a mutex in _pi_context::finalize (intel#3001)
  [SYCL] Fix various compilation warnings in plugins (intel#2979)
  [SYCL][ESIMD] Add simd class conversion ctor and operator (intel#3028)
  [sycl-post-link][NFC] Use range-based for loop. (intel#3033)
  [SYCL][NFC] Fix warning in self-build (intel#3023)
  [NFC] Fix sycl-post-link tests to avoid potential race (intel#3031)
  [SYCL][CUDA] Add missing barrier to collectives (intel#2990)
  [SYCL] Make Intel attributes consistent with clang attributes. (intel#3022)
  [SYCL] Bump SYCL minor version (intel#3026)
  [SYCL][Doc] Added requirement on reference to test PR in commit message (intel#3010)
  [SYCL] Put constant initializer list data in non-generic addr space. (intel#3005)
  [SYCL][L0] Fix memory leak in PiDeviceCache and ZeCommandList (intel#2974)
  [SYCL] Fix detection of free function calls (intel#3003)
  [SYCL][NFC] Clean up the builder_dir argument description (intel#3021)
  [SYCL][ESIMD] Fix LowerESIMD crash on a scalar fptoui LLVM instruction (intel#2699)
  [NFC] Remove redundant call to getMainExecutable() (intel#3018)
  ...
bader added a commit to bader/llvm that referenced this pull request Jan 29, 2021
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.

4 participants