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

[LLVM] upgrade to LLVM 11 #37842

Merged
merged 8 commits into from
Oct 13, 2020
Merged

[LLVM] upgrade to LLVM 11 #37842

merged 8 commits into from
Oct 13, 2020

Conversation

vchuravy
Copy link
Member

@vchuravy vchuravy commented Oct 1, 2020

Based on LLVM 11-rc5. Still some patch maintenance to do.

TODO:

  • D75072

@vchuravy vchuravy mentioned this pull request Oct 7, 2020
@vchuravy vchuravy marked this pull request as ready for review October 9, 2020 22:25
@@ -1306,6 +1306,8 @@ bool GCChecker::evalCall(const CallExpr *CE,
// (globals should not be invalidated, etc), hence the use of evalCall.
#if LLVM_VERSION_MAJOR >= 9
const CallExpr *CE = dyn_cast<CallExpr>(Call.getOriginExpr());
if (!CE)
Copy link
Member Author

Choose a reason for hiding this comment

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

As an example I observed:

(gdb) p Call.getOriginExpr()->dump()
CXXConstructExpr 0x555557d1e910 'class llvm::APInt' 'void (void)'
$3 = void

@Keno does this change look ok?

@vchuravy
Copy link
Member Author

vchuravy commented Oct 11, 2020

@Keno & @vtjnash The x86 Linux failure is peculiar:

What I observed is that we want to perfom a ccall to jl_alloc_array_1d, but instead jump into the middle off jl_alloc_array_3d.
Dumping the LLVM module IR shows that we embed the correct pointer. Running with --debug=dyld and dissasembling the object file before and after relocations. Shows that we changed from the correct address to a wrongly offset address.

As an example: The emitted e815028ef7 changes to e8b1d9590c. ntoh(0x15028ef7) == 0xf78e0215 which is the location we want and IIUC opcode e8 correctly 0xeb342994 + ntoh(0xb1d9590c) == 0xf78e0345, which is consistent with us jumping past jl_alloc_array_1d and landing in the middle of jl_alloc_array_3d.

Looking at RuntimeDyld: (different binary so address are different)

=> 0xeae8698f <+271>:   call   0xf7724335 <jl_alloc_array_3d+106> 
b RuntimeDyldELF.cpp:935 if RE.Offset == 272

resolveRelocation -> Addend: 0xf7724331

(rr) p/x &(RE.Addend)
(rr) w *0x5916a704
(rr) rc
> addRelocationForSymbol
> RECopy.Addend += SymInfo.getOffset();

b processRelocationRef
(rr) rc
in processRelocationRef: L1210

Value.SymbolName == ""
Value.Addend == 0
Offset == 272

in processRelocationRef: L1772
Value.Addend == 0xf7724205


(rr) disas jl_alloc_array_1d                                                                   
Dump of assembler code for function jl_alloc_array_1d:
   0xf7724209 <+0>:     endbr32 
   0xf772420d <+4>:     push   %ebp
   0xf772420e <+5>:     mov    %esp,%ebp
  1. SymbolName will be the empty string
  2. We will call processSimpleRelocation https://github.com/llvm/llvm-project/blame/176249bd6732a8044d457092ed932768724a6f06/llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp#L990 and that only checks if SymbolName is 0x0.
  3. addRelocationForSymbol https://github.com/llvm/llvm-project/blob/176249bd6732a8044d457092ed932768724a6f06/llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp#L936 comes in and will fudge the address.

The peculiar thing is that this behavior is positively old. @Keno you touched some of this code so maybe you have a in insight what the correct behaviour should be? Right now it is unclear to me what changed between LLVM 10 and LLVM 11.

Checking in https://github.com/llvm/llvm-project/blob/176249bd6732a8044d457092ed932768724a6f06/llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp#L930 for SymbolName.empty() let's me compile Julia, but makes me wonder why the empty symbol is in the GlobalSymbolTable.

-- edit:
Indeed we are entering an empty string here: https://github.com/llvm/llvm-project/blame/176249bd6732a8044d457092ed932768724a6f06/llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp#L343

vchuravy and others added 8 commits October 12, 2020 17:35
The command line parser used to be tolerant of this oddity, since the first argument is supposed to use different escaping rules than the other arguments. But now we are using the same rules for all arguments, so it no longer worked.
@vchuravy
Copy link
Member Author

vchuravy commented Oct 12, 2020

LLVM 11 final landed this morning, absent any CI issue this is ready to land.

@vchuravy
Copy link
Member Author

@nanosoldier runbenchmarks(ALL, vs = ":master")

@vchuravy
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -c 1,2,3,4 -k on`, ProcessExited(2)) [2]

Logs and partial data can be found here
cc @christopher-dG

@christopher-dG
Copy link
Member

Oops, forgot to restart the server after I merged my fix.

@nanosoldier runbenchmarks(ALL, vs = ":master")

@christopher-dG
Copy link
Member

christopher-dG commented Oct 12, 2020

@nanosoldier runbenchmarks(ALL, vs = ":master")

oops

edit: Oh wait, I'm not permitted to trigger this since I'm not in JuliaLang am I?

@vchuravy
Copy link
Member Author

@nanosoldier runbenchmarks(ALL, vs = ":master")

@vchuravy vchuravy added external dependencies Involves LLVM, OpenBLAS, or other linked libraries merge me PR is reviewed. Merge when all tests are passing labels Oct 12, 2020
@nanosoldier
Copy link
Collaborator

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

@vchuravy vchuravy merged commit 21e7486 into master Oct 13, 2020
@vchuravy vchuravy deleted the vc/llvm11 branch October 13, 2020 02:27
@nanosoldier
Copy link
Collaborator

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

@KristofferC
Copy link
Member

Looks like the commit doesn't exist.

@maleadt
Copy link
Member

maleadt commented Oct 13, 2020

@KristofferC
Copy link
Member


["misc", "afoldl", "Int"] | 13.18 (5%) ❌ | 1.00 (1%)

["array", "index", "(\"sumcartesian_view\", \"BitMatrix\")"] | 2.18 (50%) ❌ | 1.00 (1%)


might be interesting

@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external dependencies Involves LLVM, OpenBLAS, or other linked libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants