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.

Sign up for GitHub

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

Jump to bottom

Optionally use NewPM for optimization #46176

Merged
merged 2 commits into from
Sep 7, 2022
Merged

Optionally use NewPM for optimization #46176

merged 2 commits into from
Sep 7, 2022

Conversation

pchintalapudi
Copy link
Member

This continues from #46175 and enables NewPM on all of our optimization pipelines, unless ASAN is active. This is because the combination of ASAN, our JIT, and NewPM results in a recursive lookup during codegen, which triggers an assert for our memory manager.

This is the other half of #44365.

@pchintalapudi pchintalapudi requested a review from vchuravy July 26, 2022 06:53
@pchintalapudi pchintalapudi added compiler:codegen Generation of LLVM IR and native code compiler:llvm For issues that relate to LLVM labels Jul 26, 2022
@pchintalapudi pchintalapudi mentioned this pull request Jul 27, 2022
Merged
Base automatically changed from pc/newpm_pipeline to master July 30, 2022 04:11
@pchintalapudi pchintalapudi force-pushed the pc/newpm_pipeline2 branch 2 times, most recently from 283d735 to 78dabb2 Compare July 30, 2022 04:27
@pchintalapudi pchintalapudi marked this pull request as ready for review July 30, 2022 04:27
@vchuravy
Copy link
Member

@nanosoldier runtests(ALL, vs = ":master", buildflags=["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"], vs_buildflags=["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"])

@vchuravy
Copy link
Member

@nanosoldier runbenchmarks(!"scalar", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vchuravy
Copy link
Member

With the performance regression that nanosoldier sees it might be better to have a flag that turns this on?

@pchintalapudi
Copy link
Member Author

There is a flag (JL_USE_NEW_PM) that can be used to disable new PM.

@vchuravy
Copy link
Member

There is a flag (JL_USE_NEW_PM) that can be used to disable new PM.

I was thinking a runtime flag not a compile time flag so that's easy to switch, but maybe we can just expose it in Make.inc so that it is easy to set

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@vchuravy
Copy link
Member

vchuravy commented Aug 8, 2022

Seems:

Instruction does not dominate all uses!
  %3506 = load atomic {} addrspace(10)*, {} addrspace(10)* addrspace(11)* %3505 unordered, align 8, !dbg !959, !tbaa !16, !nonnull !6, !dereferenceable !22, !align !14
  %5400 = addrspacecast {} addrspace(10)* %3506 to {} addrspace(11)*, !dbg !1022
LLVM ERROR: Broken function found, compilation aborted!

[526] signal (6): Aborted
in expression starting at /home/pkgeval/.julia/packages/MLUtils/aThik/src/parallel.jl:119
gsignal at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
abort at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
_ZN4llvm18report_fatal_errorERKNS_5TwineEb at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm18report_fatal_errorEPKcb at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm12VerifierPass3runERNS_8FunctionERNS_15AnalysisManagerIS1_JEEE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
run at /workspace/srcdir/usr/include/llvm/IR/PassManagerInternal.h:88
_ZN4llvm11PassManagerINS_8FunctionENS_15AnalysisManagerIS1_JEEEJEE3runERS1_RS3_ at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
run at /workspace/srcdir/usr/include/llvm/IR/PassManagerInternal.h:88
_ZN4llvm27ModuleToFunctionPassAdaptor3runERNS_6ModuleERNS_15AnalysisManagerIS1_JEEE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
run at /workspace/srcdir/usr/include/llvm/IR/PassManagerInternal.h:88
_ZN4llvm11PassManagerINS_6ModuleENS_15AnalysisManagerIS1_JEEEJEE3runERS1_RS3_ at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
run at /workspace/srcdir/src/pipeline.cpp:589
operator() at /workspace/srcdir/src/jitlayers.cpp:971 [inlined]
withModuleDo<(anonymous namespace)::OptimizerT::operator()(llvm::orc::ThreadSafeModule, llvm::orc::MaterializationResponsibility&)::<lambda(llvm::Module&)> > at /workspace/srcdir/usr/include/llvm/ExecutionEngine/Orc/ThreadSafeModule.h:136 [inlined]
operator() at /workspace/srcdir/src/jitlayers.cpp:943 [inlined]
CallImpl<(anonymous namespace)::OptimizerT> at /workspace/srcdir/usr/include/llvm/ADT/FunctionExtras.h:222
_ZN4llvm3orc16IRTransformLayer4emitESt10unique_ptrINS0_29MaterializationResponsibilityESt14default_deleteIS3_EENS0_16ThreadSafeModuleE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
emit at /workspace/srcdir/src/jitlayers.cpp:523
_ZN4llvm3orc31BasicIRLayerMaterializationUnit11materializeESt10unique_ptrINS0_29MaterializationResponsibilityESt14default_deleteIS3_EE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm3orc19MaterializationTask3runEv at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm6detail18UniqueFunctionBaseIvJSt10unique_ptrINS_3orc4TaskESt14default_deleteIS4_EEEE8CallImplIPFvS7_EEEvPvRS7_ at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession12dispatchTaskESt10unique_ptrINS0_4TaskESt14default_deleteIS3_EE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession22dispatchOutstandingMUsEv at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession17OL_completeLookupESt10unique_ptrINS0_21InProgressLookupStateESt14default_deleteIS3_EESt10shared_ptrINS0_23AsynchronousSymbolQueryEESt8functionIFvRKNS_8DenseMapIPNS0_8JITDylibENS_8DenseSetINS0_15SymbolStringPtrENS_12DenseMapInfoISF_vEEEENSG_ISD_vEENS_6detail12DenseMapPairISD_SI_EEEEEE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm3orc25InProgressFullLookupState8completeESt10unique_ptrINS0_21InProgressLookupStateESt14default_deleteIS3_EE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession19OL_applyQueryPhase1ESt10unique_ptrINS0_21InProgressLookupStateESt14default_deleteIS3_EENS_5ErrorE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession6lookupENS0_10LookupKindERKSt6vectorISt4pairIPNS0_8JITDylibENS0_19JITDylibLookupFlagsEESaIS8_EENS0_15SymbolLookupSetENS0_11SymbolStateENS_15unique_functionIFvNS_8ExpectedINS_8DenseMapINS0_15SymbolStringPtrENS_18JITEvaluatedSymbolENS_12DenseMapInfoISI_vEENS_6detail12DenseMapPairISI_SJ_EEEEEEEEESt8functionIFvRKNSH_IS6_NS_8DenseSetISI_SL_EENSK_IS6_vEENSN_IS6_SV_EEEEEE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession6lookupERKSt6vectorISt4pairIPNS0_8JITDylibENS0_19JITDylibLookupFlagsEESaIS7_EERKNS0_15SymbolLookupSetENS0_10LookupKindENS0_11SymbolStateESt8functionIFvRKNS_8DenseMapIS5_NS_8DenseSetINS0_15SymbolStringPtrENS_12DenseMapInfoISK_vEEEENSL_IS5_vEENS_6detail12DenseMapPairIS5_SN_EEEEEE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession6lookupERKSt6vectorISt4pairIPNS0_8JITDylibENS0_19JITDylibLookupFlagsEESaIS7_EENS0_15SymbolStringPtrENS0_11SymbolStateE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession6lookupENS_8ArrayRefIPNS0_8JITDylibEEENS0_15SymbolStringPtrENS0_11SymbolStateE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession6lookupENS_8ArrayRefIPNS0_8JITDylibEEENS_9StringRefENS0_11SymbolStateE at /opt/julia/bin/../lib/julia/libLLVM-14jl.so (unknown line)
addModule at /workspace/srcdir/src/jitlayers.cpp:1186
jl_add_to_ee at /workspace/srcdir/src/jitlayers.cpp:1561

is a common cause for The process was aborted (37 packages):

@pchintalapudi
Copy link
Member Author

In the interest of getting the changes in and then enabling it after ironing out the bugs, I'm disabling newpm by default (it's still accessible via make CFLAGS=-DJL_USE_NEW_PM)

vchuravy
vchuravy reviewed Aug 11, 2022
View reviewed changes
test/boundscheck_exec.jl Outdated Show resolved Hide resolved
@pchintalapudi pchintalapudi changed the title Use NewPM for optimization unless ASAN is in effect Optionally use NewPM for optimization Aug 12, 2022
@pchintalapudi pchintalapudi force-pushed the pc/newpm_pipeline2 branch 2 times, most recently from a8c3db1 to 1e562cd Compare August 19, 2022 04:45
@pchintalapudi
Copy link
Member Author

If the tests come back clean I will merge the PR since every nontrivial change is underneath an ifdef.

@pchintalapudi
Copy link
Member Author

The win64 and win32 failures are network errors, and are unrelated to this change.

@pchintalapudi pchintalapudi merged commit 83658d2 into master Sep 7, 2022
@pchintalapudi pchintalapudi deleted the pc/newpm_pipeline2 branch September 7, 2022 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vchuravy vchuravy vchuravy left review comments

@vtjnash vtjnash vtjnash approved these changes

Assignees
No one assigned
Labels
compiler:codegen Generation of LLVM IR and native code compiler:llvm For issues that relate to LLVM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pchintalapudi @vchuravy @nanosoldier @vtjnash