Skip to content

Conversation

@islavutin
Copy link
Contributor

Minor change in config to address build issue with the latest version of Apache TVM

$(ROOT_PATH)/3rdparty/dlpack/include \
$(ROOT_PATH)/3rdparty/dmlc-core/include
$(ROOT_PATH)/3rdparty/dmlc-core/include \
$(ROOT_PATH)/3rdparty/compiler-rt
Copy link
Member

Choose a reason for hiding this comment

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

compiler-rt should be directly included using relative path, @islavutin can you point out the file that did not do that?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry @tqchen, could you expand on what you meant here?

Copy link
Member

Choose a reason for hiding this comment

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

I meant that 3rdparty/compiler-rt does not need to be part of include path, because we suppose to include the files in it via relative ones, e.g. https://github.com/apache/tvm/blob/main/src/runtime/contrib/random/mt_random_engine.cc#L32

Copy link
Member

Choose a reason for hiding this comment

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

so a fix should be change other include of the compiler rt file to relative include. We did not intend to use that particular file as a public header

Copy link
Member

Choose a reason for hiding this comment

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

Does that still hold here as this is in the apps folder? I'd assume the app is setup so it could be plucked out of the tree?

Copy link
Member

Choose a reason for hiding this comment

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

i think it still holds, because the all in one packed still requires the src tree due to the mechanism we used to build the packed runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @tqchen, I have updated the fix based on your recommendations

@junrushao
Copy link
Member

Please resolve the merge conflicts and let’s get it merged

@islavutin
Copy link
Contributor Author

Closing this PR since issue is addressed by #8705

@islavutin islavutin closed this Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants