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

[Codegen] fix bug on LLVM 10.0 #4480

Merged
merged 1 commit into from
Dec 8, 2019
Merged

[Codegen] fix bug on LLVM 10.0 #4480

merged 1 commit into from
Dec 8, 2019

Conversation

qingyunqu
Copy link
Contributor

Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

Fix llvm::CGFT_xxx to llvm::TargetMachine::CGFT_xxx.

@qingyunqu
Copy link
Contributor Author

@yzhliu

@tqchen tqchen merged commit 03a59bc into apache:master Dec 8, 2019
@tqchen
Copy link
Member

tqchen commented Dec 8, 2019

Thanks @qingyunqu !

@masahi
Copy link
Member

masahi commented Dec 9, 2019

@qingyunqu this breaks the build with the trunk as pointed out in #4386. Can you revert this change and instead fix version ifdef? Sorry I didnt test in llvm 10.0.

@qingyunqu
Copy link
Contributor Author

@masahi Could I push a new version that reduce the second and the third ifdef into one?

@masahi
Copy link
Member

masahi commented Dec 10, 2019

No, second elif should be TVM_LLVM_VERSION <= 100. The last one is for the latest llvm.

@masahi
Copy link
Member

masahi commented Dec 10, 2019

@qingyunqu where did your get your llvm 10.0? It is not released yet. Maybe you are using out of date trunk?

@qingyunqu
Copy link
Contributor Author

@masahi Maybe I used the master branch of LLVM
截屏2019-12-10下午7 32 04

@masahi
Copy link
Member

masahi commented Dec 10, 2019

Ok, that means you are using an out of date LLVM trunk. As I mentioned in #4386, LLVM recently introduced changes that break the build of TVM with the LLVM trunk. #4386 fixed that issue, but you reverted that fix in this PR.

Please send another PR that reverts this change. If you update and rebuild your LLVM, the build should succeed.

@qingyunqu
Copy link
Contributor Author

@masahi Thanks, I will send the PR immediately.

@qingyunqu
Copy link
Contributor Author

@masahi I have used svn to update my LLVM. But after rebuilding LLVM, the build of tvm without TargetMachine:: also failed. May I make a mistake or misunderstand?

@masahi
Copy link
Member

masahi commented Dec 10, 2019

A couple of points:

  • LLVM project moved to github https://github.com/llvm/llvm-project. You might want to switch to this repo from svn.
  • Make sure you have the enum CodeGenFileType in include/llvm/Support/Codegen.h (not in TargetMachine.h). This shows that CGFT_ObjectFile etc lives under the name space llvm, not llvm::TargetMachine
  • Make sure you are building TVM with the correct LLVM installation.

@qingyunqu
Copy link
Contributor Author

Thanks, I have got the latest LLVM of 10.0.0git version and found the enum CodeGenFileType. I will test the compilation on this version and then push another PR. Thanks a lot!

@qingyunqu
Copy link
Contributor Author

@masahi I have compiled tvm with 10.0.0git LLVM. There is something wrong when I run cmake:

截屏2019-12-12上午12 50 58

Then I comment the error line in FindLLVM.cmake:43. Everything becomes ok:

截屏2019-12-12上午12 55 11

Maybe there is better method to fix this?

@masahi
Copy link
Member

masahi commented Dec 11, 2019

I also remember seeing that error (but I don't remember how I fixed it). Unless you have a good solution for this, we can fix that later. If you've confirmed that you can build TVM with the LLVM trunk, please send a PR that reverts this change.

zxy844288792 pushed a commit to zxy844288792/tvm that referenced this pull request Dec 13, 2019
zxy844288792 pushed a commit to neo-ai/tvm that referenced this pull request Dec 13, 2019
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.

3 participants