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

IfStmt simplification did not handle AtomicOp correctly? #387

Closed
k-ye opened this issue Jan 15, 2020 · 4 comments
Closed

IfStmt simplification did not handle AtomicOp correctly? #387

k-ye opened this issue Jan 15, 2020 · 4 comments

Comments

@k-ye
Copy link
Member

k-ye commented Jan 15, 2020

Describe the bug

I think there is a bug when simplifying IfStmt, around how it handles atomics. See the screenshots below when running renderer.py for a difference.

If I disable the simplification when there is an AtomicOp inside IfStmt, the bug gets resolved.

I.e., change this line in simplify.cpp from

  if (is_global_write(clause[i].get()) ||

to

  if (clause[i]->is<GlobalStoreStmt>() ||

I'm running the CPU version on macOS Catalina 10.15.2.

Log/Screenshots

This is the result I got when running the renderer example on the head release 0.3.21 or the master branch 0726d49:

Screen Shot 2020-01-15 at 22 28 12

And if I patch in the fix, the rendered result will show the taichi logo

Screen Shot 2020-01-15 at 22 29 06

To Reproduce

python examples/renderer.py

@yuanming-hu
Copy link
Member

Refactoring simplify.cpp to fix this and make the correctness more trivial...

@yuanming-hu
Copy link
Member

The minimal script to reproduce this issue:

@ti.all_archs
def test_local_atomic_with_if():
  ret = ti.var(dt=ti.i32, shape=())
  ti.cfg.print_ir = True

  @ti.kernel
  def test():
    if True:
      x = 0
      x += 1
      ret[None] = x

  test()
  assert ret[None] == 1

@yuanming-hu
Copy link
Member

Fixed for now by adding

op->instance_id == alloca->instance_id)) {

in is_atomic_value_used. Will take a deeper look into the whole simplify.cpp file tomorrow to figure out more systematic solutions. Releasing v0.3.22 with these improvements. Thanks!

@yuanming-hu
Copy link
Member

Since no related issues have been raised about the simplified pass, I assume simplify.cpp works with no problem...

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

No branches or pull requests

2 participants