-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[TIR] Moved PrimExpr operator overload from op.h to expr.h #11973
[TIR] Moved PrimExpr operator overload from op.h to expr.h #11973
Conversation
7699b49
to
62f62a1
Compare
Hmm, perhaps not as straightforward as a change as I had hoped. It looks like there are already cases that implicitly rely on the I think this or a similar change would still be useful, as erroneous use of |
Changing this to a draft after further inspection. There are some cases (example) that rely on operations between |
My opinion is that we should allow implicit conversions from C++ builtin types into |
Added a PR that removes implicit conversion from |
If a compilation unit includes `<tvm/ir/expr.h>`, but does not include `<tvm/tir/op.h>`, the operator overloads for `ObjectRef` are declared, but the operator overloads for `PrimExpr` are not. In this case, any use of `expr_a == expr_b` would use `ObjectRef`'s implementation and compare reference equality of the two expressions, rather than returning a `PrimExpr` that represents the comparison. By having the operator overloads in the `<tvm/ir/expr.h>` header file, directly adjacent to the `PrimExpr` declaration, the correct overload must be available whenever the `PrimExpr` can be used. Even though this would only impact `operator==`, `operator!=`, and `operator<`, the three operators defined for `ObjectRef`, this PR moves all operator overloads to `expr.h` for consistency. The named version of the operators (e.g. `tvm::add`) do not have overloaded variants, and so they are intentionally kept in `<tvm/tir/op.h>`.
Needed to avoid ambiguity between `TVMRetValue -> bool` conversion and `TVMRetValue -> int -> PrimExpr` conversion.
Use of `std::set<Call>` had ambiguity between `operator<` by `PrimExpr` or by `ObjectRef`. The comment for `call_order_` implied that the previous usage of `std::set<Call>` was intended to have a de-duplicated list in the order of occurrence. However, the `std::set` was ordered by `ObjectRef::operator<`, not by insertion order. Switching to using a `vector` for ordering and `unordered_set` for de-duplication resolves this issue, and also removes the use of `operator<`.
b60c1d7
to
4eca8e0
Compare
And rebased onto #12010, and that clears up almost everything. I have a few additional commits on this PR, in order to resolve the last of the ambiguous operators. The change for |
Looks fine to me @Lunderberg ! |
Thank you! With that confirmation, moving this change back out from "Draft" to "Ready to Review" |
) * [TIR] Moved PrimExpr operator overload from op.h to expr.h If a compilation unit includes `<tvm/ir/expr.h>`, but does not include `<tvm/tir/op.h>`, the operator overloads for `ObjectRef` are declared, but the operator overloads for `PrimExpr` are not. In this case, any use of `expr_a == expr_b` would use `ObjectRef`'s implementation and compare reference equality of the two expressions, rather than returning a `PrimExpr` that represents the comparison. By having the operator overloads in the `<tvm/ir/expr.h>` header file, directly adjacent to the `PrimExpr` declaration, the correct overload must be available whenever the `PrimExpr` can be used. Even though this would only impact `operator==`, `operator!=`, and `operator<`, the three operators defined for `ObjectRef`, this PR moves all operator overloads to `expr.h` for consistency. The named version of the operators (e.g. `tvm::add`) do not have overloaded variants, and so they are intentionally kept in `<tvm/tir/op.h>`. * Explicitly convert TVMRetValue to bool in target.cc Needed to avoid ambiguity between `TVMRetValue -> bool` conversion and `TVMRetValue -> int -> PrimExpr` conversion. * Used vector/unordered_set to track BufferInfoExtractor::call_order_ Use of `std::set<Call>` had ambiguity between `operator<` by `PrimExpr` or by `ObjectRef`. The comment for `call_order_` implied that the previous usage of `std::set<Call>` was intended to have a de-duplicated list in the order of occurrence. However, the `std::set` was ordered by `ObjectRef::operator<`, not by insertion order. Switching to using a `vector` for ordering and `unordered_set` for de-duplication resolves this issue, and also removes the use of `operator<`. * Remove C-style cast to fix lint error
) * [TIR] Moved PrimExpr operator overload from op.h to expr.h If a compilation unit includes `<tvm/ir/expr.h>`, but does not include `<tvm/tir/op.h>`, the operator overloads for `ObjectRef` are declared, but the operator overloads for `PrimExpr` are not. In this case, any use of `expr_a == expr_b` would use `ObjectRef`'s implementation and compare reference equality of the two expressions, rather than returning a `PrimExpr` that represents the comparison. By having the operator overloads in the `<tvm/ir/expr.h>` header file, directly adjacent to the `PrimExpr` declaration, the correct overload must be available whenever the `PrimExpr` can be used. Even though this would only impact `operator==`, `operator!=`, and `operator<`, the three operators defined for `ObjectRef`, this PR moves all operator overloads to `expr.h` for consistency. The named version of the operators (e.g. `tvm::add`) do not have overloaded variants, and so they are intentionally kept in `<tvm/tir/op.h>`. * Explicitly convert TVMRetValue to bool in target.cc Needed to avoid ambiguity between `TVMRetValue -> bool` conversion and `TVMRetValue -> int -> PrimExpr` conversion. * Used vector/unordered_set to track BufferInfoExtractor::call_order_ Use of `std::set<Call>` had ambiguity between `operator<` by `PrimExpr` or by `ObjectRef`. The comment for `call_order_` implied that the previous usage of `std::set<Call>` was intended to have a de-duplicated list in the order of occurrence. However, the `std::set` was ordered by `ObjectRef::operator<`, not by insertion order. Switching to using a `vector` for ordering and `unordered_set` for de-duplication resolves this issue, and also removes the use of `operator<`. * Remove C-style cast to fix lint error
If a compilation unit includes
<tvm/ir/expr.h>
, but does not include<tvm/tir/op.h>
, the operator overloads forObjectRef
are declared, but the operator overloads forPrimExpr
are not. In this case, any use ofexpr_a == expr_b
would useObjectRef
's implementation and compare reference equality of the two expressions, rather than returning aPrimExpr
that represents the comparison. By having the operator overloads in the<tvm/ir/expr.h>
header file, directly adjacent to thePrimExpr
declaration, the correct overload must be available whenever thePrimExpr
can be used.Even though this would only impact
operator==
,operator!=
, andoperator<
, the three operators defined forObjectRef
, this PR moves all operator overloads toexpr.h
for consistency.The named version of the operators (e.g.
tvm::add
) do not have overloaded variants, and so they are intentionally kept in<tvm/tir/op.h>
.