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

Shifting operators should not adopt the same implicit conversion rule as other arithmetic operators #4795

Closed
strongoier opened this issue Apr 15, 2022 · 2 comments · Fixed by #4884
Assignees
Labels
potential bug Something that looks like a bug but not yet confirmed welcome contribution

Comments

@strongoier
Copy link
Contributor

Shifting operators like << should take the left-side type as its result type (i8 << u32 = i8) instead of following rules for other arithmetic operators (i8 + u32 = u32).

@yuanming-hu
Copy link
Member

yuanming-hu commented Apr 15, 2022

A not so minimal example to reproduce this issue (by @GCChen97, #4613):

@ti.func
def set_bit(switches, i, dt):
    switches  |= (ti.cast(1, dt) << i)
    return switches

@ti.kernel
def test():
    for _ in range(1):
        for i in range(8):
            eight_flags = ti.cast(0, ti.u8)
            eight_flags = set_bit(eight_flags, i, ti.u8)
            print(ti.cast(eight_flags, ti.i32))
test()

You will get a warning because i8 << i32 leads to i32, which is later stored into u8 (lose of precision).

@yuanming-hu
Copy link
Member

The place to look at:

void visit(BinaryOpStmt *stmt) override {

For shift operators we should use the left operand type as result type.

@FantasyVR FantasyVR moved this to Untriaged in Taichi Lang Apr 22, 2022
@FantasyVR FantasyVR moved this from Untriaged to Backlog in Taichi Lang Apr 22, 2022
Repository owner moved this from Backlog to Done in Taichi Lang Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
potential bug Something that looks like a bug but not yet confirmed welcome contribution
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants