Skip to content

Conversation

@fineg74
Copy link
Contributor

@fineg74 fineg74 commented Jun 13, 2022

This fix addresses an issue of incorrect handling of char buffers by copy_to function.
Complementary test PR intel/llvm-test-suite#1057

@fineg74 fineg74 closed this Jun 13, 2022
@fineg74 fineg74 reopened this Jun 13, 2022
@fineg74 fineg74 marked this pull request as draft June 13, 2022 20:09
@fineg74 fineg74 marked this pull request as ready for review June 14, 2022 22:45
@v-klochkov
Copy link
Contributor

The test that is supposed to verify this fix has failed here: http://llvm-ci-test2.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FLLVM_Test_Suite_Associate_CI/detail/LLVM_Test_Suite_Associate_CI/175/pipeline
Can you please take a look?

Vals.template select<4, 1>() =
Tmp.template bit_cast_view<int32_t>().template select<4, 1>(
NumChunks * ChunkSize);
block_store<int32_t, 8>(reinterpret_cast<int32_t *>(Addr) +
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work? Vals has 8 int elements where the low 4 hold some useful values, while the upper 4 keep garbage, which is block_store'd to memory.
Looks like it writes to memory that is not supposed to be written, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this and accessor version, added tests to test both versions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It uses mask to exclude data that don't need to be copied.

@fineg74
Copy link
Contributor Author

fineg74 commented Jun 22, 2022

/verify with intel/llvm-test-suite#1057

@fineg74
Copy link
Contributor Author

fineg74 commented Jun 23, 2022

The test that is supposed to verify this fix has failed here: http://llvm-ci-test2.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FLLVM_Test_Suite_Associate_CI/detail/LLVM_Test_Suite_Associate_CI/175/pipeline Can you please take a look?

Seems to work now

Remove special handling for accessor version as it is correctly handled.
@fineg74
Copy link
Contributor Author

fineg74 commented Jun 28, 2022

/verify with intel/llvm-test-suite#1057

@fineg74
Copy link
Contributor Author

fineg74 commented Jun 28, 2022

/verify with intel/llvm-test-suite#1057

1 similar comment
@fineg74
Copy link
Contributor Author

fineg74 commented Jun 28, 2022

/verify with intel/llvm-test-suite#1057

@v-klochkov v-klochkov merged commit 2b8c118 into intel:sycl Jun 30, 2022
@fineg74 fineg74 deleted the CopyToFix branch July 1, 2022 01:46
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.

2 participants