Skip to content

Refactor diagnostic to avoid circular dependencies#6692

Merged
tqchen merged 7 commits into
apache:mainfrom
rkimball:bob/diag_refactor
Oct 18, 2020
Merged

Refactor diagnostic to avoid circular dependencies#6692
tqchen merged 7 commits into
apache:mainfrom
rkimball:bob/diag_refactor

Conversation

@rkimball

Copy link
Copy Markdown
Contributor

There were circular dependencies in object.h which made use of ICHECK in Object problematic.

Comment thread include/tvm/ir/diagnostic_context.h Outdated
/*! \brief A compiler diagnostic message. */
class DiagnosticNode : public Object {
public:
/*! \brief The level. */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should improve this comment.

@tqchen

tqchen commented Oct 15, 2020

Copy link
Copy Markdown
Member

In this particular case, ICHECK perhaps should not end up in ir/diagnostic, but instead in support/logging.h, moving the marco defs there would be the simplest solution. It would also avoid the dep from runtime to ir

Comment thread include/tvm/support/diagnostic.h Outdated

namespace tvm {

extern const char* kTVM_INTERNAL_ERROR_MESSAGE;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge this file content with tvm/support/logging.h as previous content are already there

kNote = 40,
kHelp = 50,
};

@tqchen tqchen Oct 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diagnostic context should not be part of support, because right now it brings large amount of dep related to IR. keep it as in include/tvm/ir/diagnostic.h

@tqchen

tqchen commented Oct 16, 2020

Copy link
Copy Markdown
Member

Thanks @rkimball . I feel the PR can still be simplified further to avoid potential confusion of two diagnostic header files(one minimum and another built for IR). In particular we can do:

  • Keep all the content of ir/diagnostic.h as it is in IR, because they introduce a larger trunk of deps, it should be part of IR and tvm runtime should not depend on them.
  • Move all the ICHECK def into support/logging.h, where there is already some previous related definitions of checks, this is a minimum check macro that can be used in runtime

@rkimball rkimball marked this pull request as ready for review October 16, 2020 18:36
@rkimball rkimball requested review from jroesch and tqchen October 16, 2020 18:36
Comment thread include/tvm/ir/diagnostic_context.h Outdated

/*!
* \file diagnostic.h
* \file diagnostic_context.h

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that we have removed diagnostic.h I think it is better to just rename back to diagnostic.h for simplicity

@rkimball rkimball requested a review from tqchen October 16, 2020 22:32

@jroesch jroesch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@tqchen tqchen merged commit 80cc480 into apache:main Oct 18, 2020
@tqchen

tqchen commented Oct 18, 2020

Copy link
Copy Markdown
Member

Thanks @rkimball @jroesch

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Oct 29, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 2, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 4, 2020
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Dec 4, 2020
@rkimball rkimball deleted the bob/diag_refactor branch January 8, 2021 22:18
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