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

[llvm] Use GEP for array access instead of ptrtoint/inttoptr #4276

Merged
merged 3 commits into from
Feb 15, 2022

Conversation

strongoier
Copy link
Contributor

Related issue = #2590

Using ptrtoint/inttoptr for array access prevents LLVM from demoting array alloca into individual allocas, which further prevents mem2reg optimizations. After this PR, with dynamic_index=True, we can have the same runtime performance as with dynamic_index=False for old code which can only access matrices with constant indices.

@netlify
Copy link

netlify bot commented Feb 14, 2022

✔️ Deploy Preview for docsite-preview ready!

🔨 Explore the source changes: ece9d3a

🔍 Inspect the deploy log: https://app.netlify.com/sites/docsite-preview/deploys/620b172ff79809000887c413

😎 Browse the preview: https://deploy-preview-4276--docsite-preview.netlify.app

@strongoier
Copy link
Contributor Author

/benchmark

@taichi-gardener
Copy link
Contributor

current commit hash: e17f818

master commit hash: b53523c

Examples CUDA
benchmarks fluctuation threshold 5.0%
kernel improvement None
kernel regression None
end2end improvement None
end2end regression None

Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -69,7 +69,7 @@ def __init__(self, n=1, m=1, dt=None, suppress_warning=False):
self.local_tensor_proxy = impl.expr_init_local_tensor(
[len(n)], dt,
expr.make_expr_group([expr.Expr(x) for x in n]))
self.dynamic_index_stride = ti_core.data_type_size(dt)
self.dynamic_index_stride = 1
Copy link
Member

Choose a reason for hiding this comment

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

Has the meaning of dynamic_index_stride been changed (from bytes to index)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After this PR, handling dynamic indexing of matrix fields requires manipulating pointers directly, while handling dynamic indexing of local matrices only requires array indices. However, currently they share the same expression (TensorElementExpression) and statement (PtrOffsetStmt), so the stride actually stands for two different things (bytes & indices) in these two cases. A future step must be to split them up. As it will add even one more type of pointer statement, I suggest doing this while we refactor those pointer statements.

@strongoier strongoier merged commit e6b7279 into taichi-dev:master Feb 15, 2022
@strongoier strongoier deleted the dynamic-index-perf branch February 15, 2022 09:31
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

3 participants