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

[SYCL][ESIMD] Add test on simd bitwise not operator #850

Merged
merged 9 commits into from
Mar 22, 2022

Conversation

vasilytric
Copy link

No description provided.

Copy link

@yuriykoch yuriykoch left a comment

Choose a reason for hiding this comment

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

Actually, I'm not quite sure it's safe to use bitwise operators over signed integral values in general.

The reasons behind are:

  • general rule: INT13-C
  • trap values and padding bits possibility: n2218 mentions all details
  • possible existence of compiler optimizations relying on such restrictions (like the ones for arithmetical operations mentioned in cppcon 2018)

Tagging @kbobrovs for awareness

@yuriykoch
Copy link

Looks like while there is possibility of trap values and compiler optimizations in general C++17, the llvm itself states that signed integer types are guaranteed to be two's complement:

Given that information I would suggest to do the following:

  1. provide coverage for signed integral types, making it as generic as possible from the point of C++17 spec (using For_bitwise_not),
  2. but split operator_bitwise_not.cpp into the operator_bitwise_not_uint.cpp and operator_bitwise_not_sint.cpp to have specific test on signed integral types;
  3. mention all links from the discussion in operator_bitwise_not_sint.cpp description to make the future support easier

@vasilytric Feel free to state any concerns for such solution. The other possible option is to make a two's complement representation a test precondition (run assert in operator_bitwise_not_sint.cpp, so there would be no need in For_bitwise_not in such case)

- Add comments about possible UB
- Remove unnecessary comment
- Remove Unnecessary field
Because we have possibility of trap values and compiler optimizations
in general C++17.
- Update comment
- use operators namespace
@vasilytric
Copy link
Author

Looks like while there is possibility of trap values and compiler optimizations in general C++17, the llvm itself states that signed integer types are guaranteed to be two's complement:

The test has been split in two in this commit: 66e0d60

- Make some variables const
- Update log messages
- Update some comments
@vasilytric
Copy link
Author

@v-klochkov Would you mind to review this PR?

v-klochkov
v-klochkov previously approved these changes Mar 17, 2022
@vladimirlaz vladimirlaz merged commit e36c713 into intel:intel Mar 22, 2022
myler pushed a commit to myler/llvm-test-suite that referenced this pull request Jun 17, 2022
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 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.

4 participants