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

removing support in llvm 6 and below #2699

Merged
merged 6 commits into from Apr 16, 2019

Conversation

Projects
None yet
7 participants
@mortzur
Copy link
Contributor

commented Apr 10, 2019

Description:
Removing code support in LLVM package versions <= 6.
Edited: CI scripts were modified to use LLVM version 8.
The reason for skipping LLVM-7 is a problematic llvm-7 distribution for Ubuntu-16.04 (xenial), which has been built with old C++ ABI and therefore is incompatible. Locally (MacOS) tests pass also on llvm-7.

Should be followed up (in next commit) by removing all FACEBOOK_INTERNAL branches (with the specific patch number).

Testing:

  • cmake -G Ninja -DCMAKE_BUILD_TYPE=Release ../glow
    -- Found LLVM 7.0.1
    ...
    ninja test

  • cmake -G Ninja -DCMAKE_BUILD_TYPE=Release ../glow -DCMAKE_PREFIX_PATH=/usr/local/Cellar/llvm/8.0.0/
    -- Found LLVM 8.0.0
    ...
    ninja test

  • On a devserver (changes copied manually, llvm version 8):
    buck build //glow/...
    buck test //glow/...

Documentation:

Fixes #2560

@mortzur mortzur requested a review from opti-mix Apr 10, 2019

@rdzhabarov

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Fixes #2560

@rdzhabarov

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

You need to fix this https://github.com/pytorch/glow/blob/master/.circleci/build.sh#L45

Also, rebased on the latest master.

@opti-mix
Copy link
Contributor

left a comment

Overall, looks pretty good.

But it seems like you removed some conditionally compiled code that was meant for LLVM >= 8. To make sure that we do not break LLVM 8 builds with this patch, could you try to install LLVM8 locally and check that you can build Glow with it after applying your changes?

DIFunction = DIBuilder_->createFunction(
scope, F->getName(), "", file, lineNo, DIFunctionTy,
false /* internal linkage */, true /* definition */, lineNo,
llvm::DINode::FlagPrototyped, true /* isOptimized */);
#else

This comment has been minimized.

Copy link
@opti-mix

opti-mix Apr 10, 2019

Contributor

Why did you remove this snippet? This was introduced for LLVM >=8, IIRC.

This comment has been minimized.

Copy link
@mortzur

mortzur Apr 10, 2019

Author Contributor

This passed in the internal version, therefore I wrongfully assumed it passes for llvm8. It doesn't built locally. reverting all of these patches.

dbgRegistrationListener_->NotifyObjectEmitted(loadedObj, objInfo);
#else

This comment has been minimized.

Copy link
@opti-mix

opti-mix Apr 10, 2019

Contributor

Why did you remove this snippet? This was introduced for LLVM >=8, IIRC.

This comment has been minimized.

Copy link
@mortzur

mortzur Apr 10, 2019

Author Contributor

This passed in the internal version, therefore I wrongfully assumed it passes for llvm8. It doesn't built locally. reverting all of these patches.

objectLayer_(ES_,
[this](llvm::orc::VModuleKey) {
return RTDyldObjectLinkingLayer::Resources{
std::make_shared<SectionMemoryManager>(), resolver_};
},
NotifyLoadedFunctor(this)),
#else

This comment has been minimized.

Copy link
@opti-mix

opti-mix Apr 10, 2019

Contributor

Why did you remove this snippet? This was introduced for LLVM >=8, IIRC.

This comment has been minimized.

Copy link
@mortzur

mortzur Apr 10, 2019

Author Contributor

This passed in the internal version, therefore I wrongfully assumed it passes for llvm8. It doesn't built locally. reverting all of these patches.

@jfix71
Copy link
Contributor

left a comment

Cool!

@@ -135,7 +116,7 @@ GlowJIT::GlowJIT(llvm::TargetMachine &TM)
return RTDyldObjectLinkingLayer::Resources{
std::make_shared<SectionMemoryManager>(), resolver_};
}),
#elif LLVM_VERSION_MAJOR > 6
#else // LLVM_VERSION_MAJOR > 6

This comment has been minimized.

Copy link
@jfix71

jfix71 Apr 10, 2019

Contributor

This comment seems incorrect

This comment has been minimized.

Copy link
@mortzur

mortzur Apr 10, 2019

Author Contributor

Hmm. The comment refers to versions for which the following section should work.
Do you mean that I'm not actually enforcing the condition anywhere?

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Apr 10, 2019

Contributor

yes the condition is not enforced anywhere.

This comment has been minimized.

Copy link
@jfix71

jfix71 Apr 11, 2019

Contributor

I was referring to the other side of this branch not having anything to do with LLVM_VERSION_MAJOR > 6, i.e. it's #if FACEBOOK_INTERNAL && LLVM_VERSION_PATCH < 20181009 vs. this #else

@@ -300,13 +290,10 @@ void LLVMIRGen::performCodeGen() {
#if FACEBOOK_INTERNAL && LLVM_VERSION_PATCH < 20181009
getTargetMachine().addPassesToEmitFile(
PM, asmStream, llvm::TargetMachine::CodeGenFileType::CGFT_AssemblyFile);
#elif LLVM_VERSION_MAJOR > 6
#else // LLVM_VERSION_MAJOR > 6

This comment has been minimized.

Copy link
@jfix71

jfix71 Apr 10, 2019

Contributor

This comment seems incorrect.

@@ -49,18 +49,13 @@ class GlowJIT {
SymbolStringPool SSP_;
ExecutionSession ES_;
std::shared_ptr<SymbolResolver> resolver_;
#elif LLVM_VERSION_MAJOR > 6
#else // LLVM_VERSION_MAJOR > 6

This comment has been minimized.

Copy link
@jfix71

jfix71 Apr 10, 2019

Contributor

This comment seems incorrect.

@mortzur mortzur force-pushed the mortzur:remove_support_llvm_6 branch 3 times, most recently from d245578 to f625925 Apr 10, 2019

@@ -160,18 +160,19 @@ LLVMIRGen::getOrCreateFunctionDebugInfo(llvm::Function *F, llvm::DIScope *scope,
auto *DIFunctionTy = DIBuilder_->createSubroutineType(
DIBuilder_->getOrCreateTypeArray(paramTys));
// Create a debug information for the current function.
#if LLVM_VERSION_MAJOR < 8 || FACEBOOK_INTERNAL
#if LLVM_VERSION_MAJOR == 7

This comment has been minimized.

Copy link
@opti-mix

opti-mix Apr 10, 2019

Contributor

Is it OK that FACEBOOK_INTERNAL was removed?

This comment has been minimized.

Copy link
@mortzur

mortzur Apr 15, 2019

Author Contributor

fixing

@opti-mix

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

On a devserver (changes copied manually, llvm version 8):
buck build //glow/...
buck test //glow/...

Does it also build with the currently available LLVM on devservers? I think the default LLVM on devservers is not 8.0 yet, or?

@mortzur mortzur force-pushed the mortzur:remove_support_llvm_6 branch from f625925 to 8820e89 Apr 10, 2019

@mortzur

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

On a devserver (changes copied manually, llvm version 8):
buck build //glow/...
buck test //glow/...

Does it also build with the currently available LLVM on devservers? I think the default LLVM on devservers is not 8.0 yet, or?

yes and yes.

@mortzur mortzur force-pushed the mortzur:remove_support_llvm_6 branch 14 times, most recently from ec632da to c204194 Apr 10, 2019

sudo apt-get update
sudo apt-get install -y llvm-6.0 llvm-6.0-dev libpng-dev libgoogle-glog-dev
sudo apt-get install -y llvm-8 clang-8 llvm-8-dev libclang-8-dev libpng-dev libgoogle-glog-dev opencl-headers

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Apr 15, 2019

Contributor

nit: why do we need to install opencl-headers for non open cl build?

This comment has been minimized.

Copy link
@mortzur

mortzur Apr 15, 2019

Author Contributor

The DEBUG case uses openCL , installs pocl which needs these installations .
Do you think it would be better to install only for DEBUG?
Do we plan to use openCL also for non-debug cases?

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Apr 15, 2019

Contributor

I think it's cleaner just add this into install_pocl function.

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Apr 15, 2019

Contributor

Do we plan to use openCL also for non-debug cases?

We could, but no matter what all openCL builds will invoke install_pocl.

This comment has been minimized.

Copy link
@mortzur

mortzur Apr 15, 2019

Author Contributor

Fixed

llvm-link-${LLVM_VERSION_MAJOR}
llvm-link-8
llvm-link-7
llvm-link-6.0

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Apr 15, 2019

Contributor

should we get rid of 6.0 here?

@@ -117,11 +117,13 @@ TEST(Quantization, Serialize) {
testSerialization(expected);
}

#if LLVM_VERSION_MAJOR < 8

This comment has been minimized.

Copy link
@nadavrot

nadavrot Apr 15, 2019

Contributor

Is this a use case that we really want to test? If it doesn't work on LLVM8 then maybe we don't really care about this use case and can simply delete it?

This comment has been minimized.

Copy link
@mortzur

mortzur Apr 15, 2019

Author Contributor

I'm just not sure if this is a bug or a valid behavior: #2588

This comment has been minimized.

Copy link
@nadavrot

nadavrot Apr 15, 2019

Contributor

Okay.

@mortzur mortzur force-pushed the mortzur:remove_support_llvm_6 branch from c204194 to df428cd Apr 15, 2019

@mortzur mortzur force-pushed the mortzur:remove_support_llvm_6 branch from df428cd to 8c0b4f2 Apr 15, 2019

@rdzhabarov

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Have you tried running these changes on fbcode?

@opti-mix
Copy link
Contributor

left a comment

Looking good now! Thanks for figuring out how to solve all these issues with different LLVM versions!

Please squash commits if needed and may be do a last check on CI and fbcode. And it should be OK to merge.

mortzur and others added some commits Apr 9, 2019

build: only support LLVM 8
Adjust the build to use the `REQUIRED` parameter to `find_package` as we want to
drop support for LLVM 6, and LLVM 8 is the first version available built with
the C++11 ABI.

Make the llvm-link search more resilient to the future by using the
LLVM_VERSION_MAJOR variable to add the suffix.  Additionally, when building
against the build tree, fetch the exported target for llvm-link and use that.

@mortzur mortzur force-pushed the mortzur:remove_support_llvm_6 branch from 8c0b4f2 to cb334ac Apr 15, 2019

@mortzur mortzur merged commit 0f74feb into pytorch:master Apr 16, 2019

6 checks passed

ci/circleci: ASAN Your tests passed on CircleCI!
Details
ci/circleci: DEBUG Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_WITH_EXPENSIVE_TESTS Your tests passed on CircleCI!
Details
ci/circleci: SHARED Your tests passed on CircleCI!
Details
ci/circleci: TSAN Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rdzhabarov

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

One of the tests failed: 25/34 Test #28: QuantizationTest ....................Child aborted***Exception: 22.27 sec

You could check the master branch results. Can you check what's going on?
Actually there are sporadic failures in tests when pocl enabled.
@mortzur please, take a look tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.