[TVMScript] Add object path tracing to StructuralEqual #12101
Conversation
Motivation: when two IR objects fail a structural equality check, currently there is no easy way to find out which part of the IR caused the mismatch. In this PR, we modify the `StructuralEqual` infrastructure to also optionally return a pair of `ObjectPath` objects that point to the mismatch. (See apache#11977). In the upcoming PRs, we will pass these paths to the TIR printer, so that it could highlight the mismatch location nicely. Tracking issue: apache#11912
junrushao
force-pushed
the
tunip-structural-equal
branch
from
1bba135 to
4c28f15
Compare
| static void GetPathsFromAttrAddressesAndStoreMismatch(const void* lhs_address, | ||
| const void* rhs_address, | ||
| const PathTracingData* tracing_data); | ||
|
|
||
| template <typename T> | ||
| static bool CompareAttributeValues(const T& lhs, const T& rhs, | ||
| const PathTracingData* tracing_data); |
There was a problem hiding this comment.
Just curious - do you think it would make more sense to move those two methods to the cc file? The primary concern I'm having is that CompareAttributeValues is a templated method whose instantiation is all inside a cc file. If we care about visibility, we could introduce a friend class SEqualReducerHelper which has those two methods as static members
There was a problem hiding this comment.
What is the actual concern about having a function template declared in the header?
I don't have a strong opinion, can move these to a helper friend class if you prefer it that way.
There was a problem hiding this comment.
There is no concrete problem in this particular case AFAICT - because template instantiation is only defined and used in a single cc file.
On the other hand, in more generic usecases, we would prefer template instantiation being defined in header files so that it's discoverable by the compiler when multiple cc files refer to this method.
Therefore, it's somehow a personal preference (so it's subjective, not any general requirement) that I either define both instantiation and declaration in header file, or both in cc files
There was a problem hiding this comment.
I think I mostly understand your point, but I'm missing one thing: why is this specific to function templates? For example, the non-template helper function GetPathsFromAttrAddressesAndStoreMismatch just above is also private but we have to put its declaration in the header file, because it is a static function in our class (which we need because of C++ visibility rules).
Even if we go with the SEqualReducerHelper approach, we still need to leak some details in the header file because we need to either declare it as a static class or as a friend class.
There was a problem hiding this comment.
Yeah my personal (again it's just subjective) preference is that we hide anything that's not intended to be publicly used, except for non-static methods when it requires some boilerplate code (adding helper friend classes / methods). If a method is in a header file, I would prefer to document it more or less to make it easier for others to catch up
Motivation: when two IR objects fail a structural equality check, currently there is no easy way to find out which part of the IR caused the mismatch. In this PR, we modify the `StructuralEqual` infrastructure to also optionally return a pair of `ObjectPath` objects that point to the mismatch. (See apache#11977). In the upcoming PRs, we will pass these paths to the TIR printer, so that it could highlight the mismatch location nicely. Tracking issue: apache#11912
Motivation: when two IR objects fail a structural equality check, currently there is no easy way to find out which part of the IR caused the mismatch. In this PR, we modify the
StructuralEqualinfrastructure to also optionally return a pair ofObjectPathobjects that point to the mismatch. (See #11977). In the upcoming PRs, we will pass these paths to the TIR printer, so that it could highlight the mismatch location nicely.Tracking issue: #11912
cc @yelite @junrushao1994