You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/06/25 08:02:31 UTC
[GitHub] [tvm] wrongtest opened a new pull request #8338: [TIR] fix storage rewrite index remap
wrongtest opened a new pull request #8338:
URL: https://github.com/apache/tvm/pull/8338
This is a fix trial for https://github.com/apache/tvm/issues/8308 where the storage rewrite pass remap the index of a vectorized load.
The offset computation should be based on the actual allocated buffer's datatype rather than the load/store op's result datatype.
For example, if a float32*1024 ramp load is relocated to offset `4096b` of a float buffer, the offset expr should be `+4096/bytes_of(float32)` rather than `+4096/bytes_of(float32*1024)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #8338: [TIR] fix storage rewrite index remap
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #8338:
URL: https://github.com/apache/tvm/pull/8338#discussion_r663035667
##########
File path: src/tir/transforms/storage_rewrite.cc
##########
@@ -504,8 +503,13 @@ class StoragePlanRewriter : public StmtExprMutator {
return MergeNest(nest, body);
}
// Remap the index
- PrimExpr RemapIndex(DataType dtype, PrimExpr index, StorageEntry* e) {
+ PrimExpr RemapIndex(PrimExpr index, StorageEntry* e) {
Review comment:
In the current setup(our code generation), the Load and Store follows the dtype specified by the instruction, regardless of the dtype specified in allocation. So it is better to use dtype in the Node. But you are right that we should not multiply by lanes when calculating the offset
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen merged pull request #8338: [TIR] fix storage rewrite index remap
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #8338:
URL: https://github.com/apache/tvm/pull/8338
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] wrongtest commented on a change in pull request #8338: [TIR] fix storage rewrite index remap
Posted by GitBox <gi...@apache.org>.
wrongtest commented on a change in pull request #8338:
URL: https://github.com/apache/tvm/pull/8338#discussion_r663608412
##########
File path: src/tir/transforms/storage_rewrite.cc
##########
@@ -504,8 +503,13 @@ class StoragePlanRewriter : public StmtExprMutator {
return MergeNest(nest, body);
}
// Remap the index
- PrimExpr RemapIndex(DataType dtype, PrimExpr index, StorageEntry* e) {
+ PrimExpr RemapIndex(PrimExpr index, StorageEntry* e) {
Review comment:
Yes you're right! I checked some mixed dtypes and found that the type casting is done before indexing for load/store. I repush the change which modify just one line of code about lanes, can you kindly review it again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #8338: [TIR] fix storage rewrite index remap
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #8338:
URL: https://github.com/apache/tvm/pull/8338#issuecomment-874382463
Thanks @wrongtest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #8338: [TIR] fix storage rewrite index remap
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #8338:
URL: https://github.com/apache/tvm/pull/8338#issuecomment-874382463
Thanks @wrongtest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen merged pull request #8338: [TIR] fix storage rewrite index remap
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #8338:
URL: https://github.com/apache/tvm/pull/8338
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #8338: [TIR] fix storage rewrite index remap
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #8338:
URL: https://github.com/apache/tvm/pull/8338#discussion_r662525566
##########
File path: src/tir/transforms/storage_rewrite.cc
##########
@@ -504,8 +503,13 @@ class StoragePlanRewriter : public StmtExprMutator {
return MergeNest(nest, body);
}
// Remap the index
- PrimExpr RemapIndex(DataType dtype, PrimExpr index, StorageEntry* e) {
+ PrimExpr RemapIndex(PrimExpr index, StorageEntry* e) {
Review comment:
Any reason we removed the dtype here? perhaps we can limit the lanes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] wrongtest commented on a change in pull request #8338: [TIR] fix storage rewrite index remap
Posted by GitBox <gi...@apache.org>.
wrongtest commented on a change in pull request #8338:
URL: https://github.com/apache/tvm/pull/8338#discussion_r662713011
##########
File path: src/tir/transforms/storage_rewrite.cc
##########
@@ -504,8 +503,13 @@ class StoragePlanRewriter : public StmtExprMutator {
return MergeNest(nest, body);
}
// Remap the index
- PrimExpr RemapIndex(DataType dtype, PrimExpr index, StorageEntry* e) {
+ PrimExpr RemapIndex(PrimExpr index, StorageEntry* e) {
Review comment:
In my understanding the most correct dtype to use is the buffer data type itself. The `dtype` param before is load_op->dtype or store_op->dtype from context.
If the buffer dtype has non-trivial lanes like `tir.allocate([n], "float32*4")` (I'm not sure if it is possible), it seems not clear which factor to use based on original dtype param.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org