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

[IR] [lang] Support SHR operator: ti.bit_shr(x, y) #1871

Merged
merged 7 commits into from
Sep 23, 2020

Conversation

TH3CHARLie
Copy link
Collaborator

Related issue = #

Since >> is already used by SAR, SHR is implemented as a function instead of a binary op.

[Click here for the format server]


Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@TH3CHARLie TH3CHARLie marked this pull request as draft September 15, 2020 03:19
assert shr(test_num, i) == 2**(n - i)
for i in range(n):
offset = 0x100000000 if i > 0 else 0
assert shr(neg_test_num, i) == (neg_test_num + offset) >> i
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test will fail on backends other than LLVM due to unimplemented op.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This includes(but not limited to...) cc, metal and opengl.

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch... I believe on these backends SHR is not implemented (but SAR is). How about this: let's demote bit_shr into a series of three operations

  • UnaryOpStmt bit_cast into unsigned
  • BinaryOpStmt bit_sar
  • UnaryOpStmt bit_cast into signed

in this pass

void visit(BinaryOpStmt *stmt) override {

https://github.com/yuanming-hu/taichi/blob/2cc50cfaeb84c6ba4f3df7d3a0caa028daadb51f/taichi/transforms/demote_operations.cpp#L17

So that backend developers don't need to worry about SAR. This is a very late pass in the compilation process - we still need SAR in the IR for certain domain-specific optimizations.

Copy link
Collaborator

@archibate archibate Sep 18, 2020

Choose a reason for hiding this comment

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

So that backend developers don't need to worry about SAR. This is a very late pass in the compilation process - we still need SAR in the IR for certain domain-specific optimizations.

Good idea! I'll later add uint support to OpenGL so that SHR works.

EDIT: So bit_sar should act as SHR when operand is uint to make this method work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@archibate That's what I'd also like to discuss. I did some experiments, in the LLVM backend, bit_sar is implemented using CreateAShr which simply copies the MSB without considering the type. While on other backends, bit_sar is expressed with >> which will behave differently according to unsigned/signed information. I think we need to decide which kind of SAR operation we truly want, either a pure one copying the MSB or a one that is more similar to the >> in C.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for all the discussions. My two cents:

  • In the frontend >> always translates to BinaryOpType::bit_sar and ti.bit_shr always translates to BinaryOpType::bit_shr.
  • In demote_operations.cpp we convert bit_shr on signed integers into three sub-operations as discussed above. Then we only have bit_sar for the backend.
  • In the backend, since we only have bit_sar, its behavior is determined by the type of its operands.

Copy link
Collaborator Author

@TH3CHARLie TH3CHARLie Sep 21, 2020

Choose a reason for hiding this comment

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

@yuanming-hu Thanks for the clarification.

The point(or problem) I'd like to discuss is the behavior of bit_sar which bit_shr will eventually rely on. As I wrote in the above comment, in the LLVM backend, bit_sar is implemented with a CreateAShr which directly maps to a sar instruction and simply copies the MSB, that is, it will ignore the type information so casting signed to unsigned will not work since the low-level bits are not changed by type casts. In other backends (at least Metal), bit_sar is implemented using the operator >>, which will consider the type information, that is, it will map to a zext and sar when working on unsigned integers. This is creating different behaviors across backends. We should first decide what kind of behavior we want for bit_sar before we move on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's follow the Metal behavior bit_sar in backends. And provide bit_shr using demote_operations as described above.
The LLVM backend should do some branches to match the behavior of Metal, e.g.:

// on bit_sar
if (dtype == unsigned) {
  CreateAShr();
} else {
  CreateLShr();
}

Copy link
Member

@yuanming-hu yuanming-hu Sep 21, 2020

Choose a reason for hiding this comment

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

Perhaps one source of confusion is that LLVM doesn't distinguish signed/unsigned integers (so does hardware such x64). So SHR always shifts everything and SAR copies the MSB in LLVM.

In Taichi, on unsigned integers bit_shr=bit_sar and they both map to SHR in LLVM. On signed integers, bit_shr maps to SHR in LLVM and bit_sar maps to SAR in LLVM.

@codecov
Copy link

codecov bot commented Sep 22, 2020

Codecov Report

Merging #1871 into master will increase coverage by 0.65%.
The diff coverage is 56.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1871      +/-   ##
==========================================
+ Coverage   43.15%   43.80%   +0.65%     
==========================================
  Files          44       45       +1     
  Lines        6271     6171     -100     
  Branches     1084     1097      +13     
==========================================
- Hits         2706     2703       -3     
+ Misses       3395     3299      -96     
+ Partials      170      169       -1     
Impacted Files Coverage Δ
python/taichi/misc/util.py 17.48% <0.00%> (-1.70%) ⬇️
python/taichi/lang/ops.py 52.44% <33.33%> (+8.88%) ⬆️
python/taichi/lang/impl.py 66.66% <50.00%> (+2.28%) ⬆️
python/taichi/lang/matrix.py 68.94% <75.00%> (+4.59%) ⬆️
python/taichi/lang/transformer.py 82.02% <84.00%> (+0.80%) ⬆️
python/taichi/lang/kernel_arguments.py 55.88% <100.00%> (ø)
python/taichi/lang/ast_checker.py 70.58% <0.00%> (-1.64%) ⬇️
python/taichi/testing.py 75.00% <0.00%> (-0.72%) ⬇️
python/taichi/lang/linalg.py 89.33% <0.00%> (-0.67%) ⬇️
python/taichi/lang/meta.py 62.31% <0.00%> (-0.54%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67da70d...8fc51d9. Read the comment docs.

@TH3CHARLie TH3CHARLie marked this pull request as ready for review September 22, 2020 10:03
@TH3CHARLie
Copy link
Collaborator Author

I've rerun the workflow for 0ff726d and it's passed.

@yuanming-hu yuanming-hu requested a review from Hanke98 September 22, 2020 23:54
Copy link
Contributor

@Hanke98 Hanke98 left a comment

Choose a reason for hiding this comment

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

Hi, thanks a lot for your great work. Most of your code is nice and clean. I only have two advice and wrote them above. : )

taichi/lang_util.h Outdated Show resolved Hide resolved
taichi/transforms/demote_operations.cpp Outdated Show resolved Hide resolved
TH3CHARLie and others added 2 commits September 23, 2020 17:29
Co-authored-by: Jiafeng Liu <jiafengliu@zju.edu.cn>
Co-authored-by: Jiafeng Liu <jiafengliu@zju.edu.cn>
@TH3CHARLie TH3CHARLie requested a review from Hanke98 September 23, 2020 09:31
Copy link
Contributor

@Hanke98 Hanke98 left a comment

Choose a reason for hiding this comment

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

Great! Thanks very much for your great work. I learnt a lot from your code and discussion. I think we can merge this pr now!

@Hanke98 Hanke98 merged commit 8c5169f into taichi-dev:master Sep 23, 2020
@TH3CHARLie TH3CHARLie deleted the impl-shr branch September 23, 2020 12:08
@yuanming-hu yuanming-hu mentioned this pull request Sep 26, 2020
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.

None yet

5 participants