-
Notifications
You must be signed in to change notification settings - Fork 79
Some misc expr simplification #105
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
Changes from all commits
cd3448f
81dcd48
da109f2
85d846a
3e8befe
8804619
5cc53e9
26f373f
15f097f
05f97b4
5257900
01bc3f5
d91025c
977a53d
5954219
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| #include <assume.h> | ||
| #include <ir_all_nodes.h> | ||
| #include <ir_builder.h> | ||
|
|
||
| #include <vector> | ||
|
|
||
| namespace nvfuser::assume { | ||
|
|
||
| Bool* tensorsAreNotEmpty(Val* value) { | ||
| std::vector<Val*> todo{value}; | ||
| std::vector<Val*> tensor_sizes; | ||
| while (!todo.empty()) { | ||
| auto v = todo.back(); | ||
| todo.pop_back(); | ||
| if (auto ns = dynamic_cast<NamedScalar*>(v)) { | ||
| if (ns->isTensorSize()) { | ||
| tensor_sizes.emplace_back(v); | ||
| continue; | ||
| } | ||
| } | ||
| if (auto def = v->definition()) { | ||
| for (auto inp : def->inputs()) { | ||
| todo.emplace_back(inp); | ||
| } | ||
| } | ||
| } | ||
| Bool* result = nullptr; | ||
| // tensor_sizes might contain duplicate, and we should remove this duplication | ||
| std::vector<Val*> tensor_sizes_applied; | ||
| for (auto ts : tensor_sizes) { | ||
| bool is_duplicate = false; | ||
| for (auto existing : tensor_sizes_applied) { | ||
| if (existing->sameAs(ts)) { | ||
| is_duplicate = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!is_duplicate) { | ||
| tensor_sizes_applied.emplace_back(ts); | ||
| result = SimplifyingIrBuilder::andExpr( | ||
| result, SimplifyingIrBuilder::gtExpr(ts, ts->container()->zeroVal())); | ||
| } | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| } // namespace nvfuser::assume | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| #include <ir_all_nodes.h> | ||
|
|
||
| // Return boolean predicates representing the conditional you want to assume. | ||
| // The return value is typically used as the `assumptions` argument of | ||
| // `simplifyExpr` | ||
|
|
||
| namespace nvfuser::assume { | ||
|
|
||
| // Return a boolean predicate stating that all tensor sizes appearing in `value` | ||
| // are positive. Return nullptr if `value` does not depend on any tensor size. | ||
| // For example: | ||
| // tensorsAreNotEmpty(ceilDiv(T0.size[0], 5) * T0.size[1]) | ||
| // -> T0.size[0] > 0 && T0.size[1] > 0 | ||
naoyam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // tensorsAreNotEmpty(ceilDiv(i1, 5) * i2) | ||
| // -> nullptr | ||
| Bool* tensorsAreNotEmpty(Val* value); | ||
|
|
||
| } // namespace nvfuser::assume | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| #include <parallel_dimension_map.h> | ||
|
|
||
| #include <ATen/cuda/CUDAContext.h> | ||
| #include <assume.h> | ||
| #include <disjoint_set.h> | ||
| #include <expr_simplifier.h> | ||
| #include <ir_utils.h> | ||
|
|
@@ -69,7 +70,16 @@ void ParallelDimensionMap::build(Fusion* fusion) { | |
|
|
||
| // Simplify dim_map_ | ||
| for (auto& [k, v] : dim_map_) { | ||
| v = simplifyExpr(v); | ||
| // Well, this isn't really correct, but we need this assumption to better | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But doesn't this actually break if empty tensors are given? I know we haven't been very strict with empty tensors, so it's highly likely there are other things that would break with empty tensors, so this isn't a new issue. Maybe we want to add a property to
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I don't know if there is anything really breaks today. If a tensor size is 0, and it appears in the parallel dim, this usually means we want to launch 0 blocks or 0 threads, this usually means all the fusion's output are empty tensor, and the lowering of this case is already skipped. The thing that I am really worried about is, if some extent is Regarding the property for non-empty tensor in
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I didn't pay close attention that this is only for vals that represent parallel dimensions. Yes, in this case, I see no concern.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think that's too different what we already do with the index type. Granted, we do have a couple of issues with it, but all we would need to do is to assume tensors can be empty by default and allow to have the non-empty tensor only with an optional flag that should be part of
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's leave this PR as is, and figure out our strategy for it when we see a bug in empty tensor. For matmul, it strongly rely on the assumption that tensors are not empty, but I don't see any other case where this is important. But I am feeling weird about the logic "if matmul, then assume nonempty, otherwise don't assume so". Whether to assume nonempty should be based on inputs, not on whether it is matmul. |
||
| // handle non-empty cases. If this turn out to be an issue, I believe we | ||
| // then need to find a more systematic way to handle empty tensor, rather | ||
| // than just disable this assumption. | ||
| auto assume = assume::tensorsAreNotEmpty(v); | ||
| if (assume != nullptr) { | ||
| v = simplifyExpr(v, {}, {assume}); | ||
| } else { | ||
| v = simplifyExpr(v); | ||
| } | ||
| } | ||
|
|
||
| // Compute exact_types_ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tensor_sizescan contain tensor size named scalars redundantly. Are you assuming the redundancy should be eliminated by the expr simplification?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching! They will not be eliminated by expr simplifier, because the
assumptionsargument are not simplified. I don't think this will cause any issue, but still, we should get rid of it. I will update this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed