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

[TIR] Moved PrimExpr operator overload from op.h to expr.h #11973

Merged
merged 4 commits into from
Jul 18, 2022

Conversation

Lunderberg
Copy link
Contributor

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>.

@Lunderberg Lunderberg changed the title [TIR] Moved PrimFunc operator overload from op.h to expr.h [TIR] Moved PrimExpr operator overload from op.h to expr.h Jun 30, 2022
@Lunderberg Lunderberg force-pushed the primfunc_operators_in_expr_h branch 2 times, most recently from 7699b49 to 62f62a1 Compare June 30, 2022 20:09
@Lunderberg
Copy link
Contributor Author

Hmm, perhaps not as straightforward as a change as I had hoped. It looks like there are already cases that implicitly rely on the PrimExpr operator overloads not being present, and break when these overloads are present. So far, all of the cases I've found are when using the Integer subclass. If the PrimExpr operator overloads are in scope, then there is ambiguity whether Integer(5) + 10 should convert the 10 into a PrimExpr to use PrimExpr operator+(PrimExpr, PrimExpr), or whether Integer(5) should use the implicit conversation to int64_t to use the builtin int64_t operator+(int64_t, int64_t).

I think this or a similar change would still be useful, as erroneous use of ObjectRef operator== on a PrimExpr could silently change the intended meaning of an expression, but it will require also making sure that Integer maintains its current behavior, regardless of which header files are included.

@Lunderberg
Copy link
Contributor Author

Changing this to a draft after further inspection. There are some cases (example) that rely on operations between Integer instances and integer literals to return a PrimExpr, and others that rely on those same expressions returning a int64_t (example). For now, changing this to a draft for whenever I can return to it.

@Lunderberg Lunderberg marked this pull request as draft June 30, 2022 21:13
@kparzysz-quic
Copy link
Contributor

[...] and others that rely on those same expressions returning a int64_t

My opinion is that we should allow implicit conversions from C++ builtin types into PrimExpr, but make the reverse conversions explicit.

@kparzysz-quic
Copy link
Contributor

Added a PR that removes implicit conversion from Integer to int64_t (linked above).

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<`.
@Lunderberg Lunderberg force-pushed the primfunc_operators_in_expr_h branch from b60c1d7 to 4eca8e0 Compare July 6, 2022 16:18
@Lunderberg
Copy link
Contributor Author

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 BufferInfoExtractor::call_order_ avoided the implicit use of ObjectRef::operator<, but did require more than just a syntax change. @manupa-arm , could you take a glance at it and make sure that this change follows your intention? Based on the docstring for BufferInfoExtract::call_order_, it looks like this std::set was intended to contain Call objects in order of occurrence and the use of ObjectRef::operator< was unintentional, but I'd like to verify.

@manupak
Copy link
Contributor

manupak commented Jul 8, 2022

Looks fine to me @Lunderberg !

@Lunderberg Lunderberg marked this pull request as ready for review July 8, 2022 16:32
@Lunderberg
Copy link
Contributor Author

Lunderberg commented Jul 8, 2022

Thank you! With that confirmation, moving this change back out from "Draft" to "Ready to Review"

@Lunderberg Lunderberg merged commit 9c7aaac into apache:main Jul 18, 2022
@Lunderberg Lunderberg deleted the primfunc_operators_in_expr_h branch August 1, 2022 15:11
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
)

* [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
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
)

* [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants