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

[Lang] Simplify dense.bitmasked to bitmasked #670

Merged
merged 4 commits into from
Mar 28, 2020
Merged

Conversation

k-ye
Copy link
Member

@k-ye k-ye commented Mar 28, 2020

Related issue = #663

Also fixed a bug where the SNode type is not set correctly on Metal:

la.append("{}.type = {};", var_name, meta.num_slots);

[Click here for the format server]

@k-ye k-ye requested a review from yuanming-hu March 28, 2020 09:23
@k-ye
Copy link
Member Author

k-ye commented Mar 28, 2020

I have a further question regarding this test

@ti.archs_support_sparse
def test_bitmasked():
x = ti.var(ti.f32)
s = ti.var(ti.i32)
n = 128
@ti.layout
def place():
ti.root.dense(ti.i, n).bitmasked().dense(ti.i, n).place(x)
ti.root.place(s)
@ti.kernel
def func():
for i in x:
s[None] += 1
x[0] = 1
x[127] = 1
x[256] = 1
x[257] = 1
func()
assert s[None] == 256

This test checks the number of iterations over the leaf SNode x. If we modify it to ti.root.bitmasked(ti.i, n).bitmasked(ti.i, n).place(x), then on CPU, it is 256. On Metal, however, it is 4 (only those being activated are iterated).

Should this be made as part of Taichi's semantics eventually? Or should we just treat this as an implementation detail? Personally I prefer the former, as it won't surprise the users. That said, this might restrict each backend's implementation.

@archibate
Copy link
Collaborator

Maybe two dense(128) caused this? The actual size of x is 128x128, however the second dim is not bitmasked, so the result is 128x2 (0 and 127 is one group, 256 and 257 is another group)

@k-ye
Copy link
Member Author

k-ye commented Mar 28, 2020

Maybe two dense(128) caused this? The actual size of x is 128x128, however the second dim is not bitmasked, so the result is 128x2 (0 and 127 is one group, 256 and 257 is another group)

Yep, the behavior of bitmasked.dense is the same. But bitmasked.bitmasked are different.

@yuanming-hu
Copy link
Member

Your Metal implementation is correct. On CPUs, bitmasked that directly includes place is not correctly implemented. I assumed the parent of place must be dense. I'll fix this: #671

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

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

LGTM!

@yuanming-hu
Copy link
Member

yuanming-hu commented Mar 28, 2020

Should this be made as part of Taichi's semantics eventually?

Yes! The ultimate goal of Taichi sparse programming is to make algorithms fully independent of data structures. Assuming the finest level must be a dense node actually harms, since the data mask now has a granularity of the dense node dim (e.g. 4x4x8 if the dense node has this dim, which is data-structure-dependent) instead of 1x1x1.

1x1x1 sparsity mask is the only option that is independent of the data structure.

@yuanming-hu yuanming-hu merged commit 4443534 into taichi-dev:master Mar 28, 2020
@k-ye k-ye deleted the bm branch March 28, 2020 14:47
archibate pushed a commit to archibate/taichi that referenced this pull request Mar 31, 2020
* [Lang] Simplify dense.bitmasked to bitmasked

* fix tests

* put back

* [skip ci] enforce code format

Co-authored-by: Taichi Gardener <[email protected]>
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.

4 participants