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

Prepare for LLVM 9 update #62474

Merged
merged 8 commits into from
Jul 10, 2019
Merged

Prepare for LLVM 9 update #62474

merged 8 commits into from
Jul 10, 2019

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Jul 7, 2019

Main changes:

  • In preparation for opaque pointer types, the byval attribute now takes a type. As such, the argument type needs to be threaded through to the function/callsite attribute application logic.
  • On ARM the +fp-only-sp and +d16 features have become -fp64 and -d32. I've switched the target definitions to use the new names, but also added bidirectional emulation so either can be used on any LLVM version for backwards compatibility.
  • The datalayout can now specify function pointer alignment. In particular on ARM Fi8 is specified, which means that function pointer alignment is independent of function alignment. I've added this to our datalayouts to match LLVM (which is something we check) and strip the fnptr alignment for older LLVM versions.
  • The fmul/fadd reductions now always respect the accumulator (including for unordered reductions), so we should pass the identity instead of undef.

Open issues:

  • https://reviews.llvm.org/D62106 causes linker errors with ld.bdf due to https://sourceware.org/bugzilla/show_bug.cgi?id=24784. To avoid this I've enabled RelaxELFRelocations, which results in a GOTPCRELX relocation for __tls_get_addr and avoids the issue. However, this is likely not acceptable because relax relocations are not supported by older linker versions. We may need an LLVM option to keep using PLT for __tls_get_addr despite RtLibUseGOT.

The corresponding llvm-project PR is rust-lang/llvm-project#19.

r? @ghost

@nikic
Copy link
Contributor Author

nikic commented Jul 7, 2019

@bors try

@bors
Copy link
Contributor

bors commented Jul 7, 2019

⌛ Trying commit bf2edb0 with merge 3b203dc...

bors added a commit that referenced this pull request Jul 7, 2019
[WIP] Update to LLVM 9

Main changes:

 * In preparation for opaque pointer types, the `byval` attribute now takes a type. As such, the argument type needs to be threaded through to the function/callsite attribute application logic.
 * On ARM the `+fp-only-sp` and `+d16` features have become `-fp64` and `-d32`. I've switched the target definitions to use the new names, but also added bidirectional emulation so either can be used on any LLVM version for backwards compatibility.
 * The datalayout can now specify function pointer alignment. In particular on ARM `Fi8` is specified, which means that function pointer alignment is independent of function alignment. I've added this to our datalayouts to match LLVM (which is something we check) and strip the fnptr alignment for older LLVM versions.
 * The fmul/fadd reductions now always respect the accumulator (including for unordered reductions), so we should pass the identity instead of undef.

Open issues:

 * https://reviews.llvm.org/D62106 causes linker errors with ld.bdf due to https://sourceware.org/bugzilla/show_bug.cgi?id=24784. To avoid this I've enabled `RelaxELFRelocations`, which results in a GOTPCRELX relocation for `__tls_get_addr` and avoids the issue. However, this is likely not acceptable because relax relocations are not supported by older linker versions. We may need an LLVM option to keep using PLT for `__tls_get_addr` despite `RtLibUseGOT`.

r? @ghost
@bors
Copy link
Contributor

bors commented Jul 8, 2019

💔 Test failed - checks-azure

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 8, 2019
@mati865
Copy link
Contributor

mati865 commented Jul 8, 2019

/checkout/src/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_posix.cc:349:59: error: use of undeclared identifier 'O_CLOEXEC'
      internal_open(shmname, O_RDWR | O_CREAT | O_TRUNC | O_CLOEXEC, S_IRWXU));
                                                          ^
1 error generated.

O_CLOEXEC is available since Linux 2.6.23 but ancient CentoOS 5 used by the CI ships with Linux 2.6.18.

@nikic
Copy link
Contributor Author

nikic commented Jul 8, 2019

@bors try

I wasn't able to get the docker container running locally ... let's see if fixing the O_CLOEXEC is enough.

@bors
Copy link
Contributor

bors commented Jul 8, 2019

⌛ Trying commit 39c7a58c39f215553ef0518ee88703eb86385287 with merge 82310f68ee567093e06ad767cc0d3cb6127457ba...

@alexcrichton
Copy link
Member

This is looking great to me, thanks @nikic! I'm not entirely certain if the munging of features for ARM is worth it for older LLVM versions, but it seems pretty localized so not too bad to carry for a bit. I suspect these aren't the only feature names that have changed over time...

@bors
Copy link
Contributor

bors commented Jul 8, 2019

☀️ Try build successful - checks-azure, checks-travis
Build commit: 82310f68ee567093e06ad767cc0d3cb6127457ba

@nikic
Copy link
Contributor Author

nikic commented Jul 8, 2019

I'm not entirely certain if the munging of features for ARM is worth it for older LLVM versions, but it seems pretty localized so not too bad to carry for a bit. I suspect these aren't the only feature names that have changed over time...

I've added this bit of emulation because these two features are used in some of the target definitions we ship, and I wasn't sure whether it's okay to make them incompatible with system LLVM.

@rust-timer build 82310f68ee567093e06ad767cc0d3cb6127457ba

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Success: Queued 82310f68ee567093e06ad767cc0d3cb6127457ba with parent 78ca1bd, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 82310f68ee567093e06ad767cc0d3cb6127457ba, comparison URL.

@mati865
Copy link
Contributor

mati865 commented Jul 9, 2019

Note for people observating this: there are more optimisations in LLVM 9 so instruction count increase in opt jobs is expected.
To measure performance of the resulting binaries somebody would have to run lolbench.

@nikic
Copy link
Contributor Author

nikic commented Jul 9, 2019

The compile-time regressions for kekkac-opt and cranelift-codegen-opt at least look fairly bad though and should be investigated. Maybe there's some low hanging fruit.

@alexcrichton
Copy link
Member

Our compatibility with the system LLVM is somewhat murkily defined. We haven't really ever supported it with 100% fidelity, but getting most tests to pass on "big targets" typically is necessary. For perhaps niche arm platforms and/or general platform supoprt 100% fidelity isn't required. In that sense it's not a bad thing to work with system LLVM's, but it's not always necessary since distributions (AFAIK), the primary use case, rarely cover the cross-compilation story affecting many of these targets.

@cuviper
Copy link
Member

cuviper commented Jul 9, 2019

since distributions (AFAIK), the primary use case, rarely cover the cross-compilation story affecting many of these targets.

On Fedora/RHEL we don't cross-compile, but we do still care about some tier-2 platforms natively.

@nikic
Copy link
Contributor Author

nikic commented Jul 9, 2019

The compile-time regressions for kekkac-opt and cranelift-codegen-opt at least look fairly bad though and should be investigated. Maybe there's some low hanging fruit.

I checked keccak. Looks like the main regression there is in the SLP vectorizer, which went from 0.2s to something like 1.5s. BoUpSLP::getSpillCost() is the hottest LLVM function in the profile at around 6.5% overall.

@alexcrichton
Copy link
Member

This seems like a great set of changes to land in the meantime even if we're hunting down LLVM issues, @nikic would you be up for landing everything here except the submodule update? We could then do that perhaps closer to the LLVM release or after we're confident in the current state of things.

@nikic nikic changed the title [WIP] Update to LLVM 9 Prepare for LLVM 9 update Jul 9, 2019
@nikic
Copy link
Contributor Author

nikic commented Jul 9, 2019

This seems like a great set of changes to land in the meantime even if we're hunting down LLVM issues, @nikic would you be up for landing everything here except the submodule update? We could then do that perhaps closer to the LLVM release or after we're confident in the current state of things.

Sounds reasonable. I've now dropped the submodule update, as well as the SubtargetSubTypeKV change and the byval codegen test update. I think those two need to happen together with the LLVM update itself.

@nikic
Copy link
Contributor Author

nikic commented Jul 9, 2019

I've landed llvm/llvm-project@5ca39e8 to mitigate a large part of the SLP perf problem, though it doesn't fix the root cause.

@alexcrichton
Copy link
Member

@bors: r+

FWIW that perf run looks excellent to me, so especially if you've mitigated the biggest regression (at least to some extent even if not the fullest) I'd be down for updating!

@bors
Copy link
Contributor

bors commented Jul 9, 2019

📌 Commit ac56025 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 9, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 10, 2019
Prepare for LLVM 9 update

Main changes:

 * In preparation for opaque pointer types, the `byval` attribute now takes a type. As such, the argument type needs to be threaded through to the function/callsite attribute application logic.
 * On ARM the `+fp-only-sp` and `+d16` features have become `-fp64` and `-d32`. I've switched the target definitions to use the new names, but also added bidirectional emulation so either can be used on any LLVM version for backwards compatibility.
 * The datalayout can now specify function pointer alignment. In particular on ARM `Fi8` is specified, which means that function pointer alignment is independent of function alignment. I've added this to our datalayouts to match LLVM (which is something we check) and strip the fnptr alignment for older LLVM versions.
 * The fmul/fadd reductions now always respect the accumulator (including for unordered reductions), so we should pass the identity instead of undef.

Open issues:

 * https://reviews.llvm.org/D62106 causes linker errors with ld.bdf due to https://sourceware.org/bugzilla/show_bug.cgi?id=24784. To avoid this I've enabled `RelaxELFRelocations`, which results in a GOTPCRELX relocation for `__tls_get_addr` and avoids the issue. However, this is likely not acceptable because relax relocations are not supported by older linker versions. We may need an LLVM option to keep using PLT for `__tls_get_addr` despite `RtLibUseGOT`.

The corresponding llvm-project PR is rust-lang/llvm-project#19.

r? @ghost
bors added a commit that referenced this pull request Jul 10, 2019
Rollup of 5 pull requests

Successful merges:

 - #61853 (Emit warning when trying to use PGO in conjunction with unwinding on …)
 - #62278 (Add Iterator::partition_in_place() and is_partitioned())
 - #62283 (Target::arch can take more than listed options)
 - #62393 (Fix pretty-printing of `$crate` (take 4))
 - #62474 (Prepare for LLVM 9 update)

Failed merges:

r? @ghost
@RalfJung
Copy link
Member

The datalayout can now specify function pointer alignment. In particular on ARM Fi8 is specified, which means that function pointer alignment is independent of function alignment. I've added this to our datalayouts to match LLVM (which is something we check) and strip the fnptr alignment for older LLVM versions.

Is that something Miri should add a check for when interpreting, to catch UB due to unaligned functions or so? I don't understand the distinction between "function alignment" and "function pointer alignment".

Currently, Miri treats function pointers as requiring alignment 1, i.e., there is no restriction.

@nikic
Copy link
Contributor Author

nikic commented Jul 10, 2019

The datalayout can now specify function pointer alignment. In particular on ARM Fi8 is specified, which means that function pointer alignment is independent of function alignment. I've added this to our datalayouts to match LLVM (which is something we check) and strip the fnptr alignment for older LLVM versions.

Is that something Miri should add a check for when interpreting, to catch UB due to unaligned functions or so? I don't understand the distinction between "function alignment" and "function pointer alignment".

Currently, Miri treats function pointers as requiring alignment 1, i.e., there is no restriction.

On most architectures function pointers have the same alignment as functions. On ARM, function pointers additionally store the ARM/Thumb mode in the low bit, so function pointers effectively become unaligned.

I don't know whether Miri can do anything interesting here...

@bors bors merged commit ac56025 into rust-lang:master Jul 10, 2019
@RalfJung
Copy link
Member

On most architectures function pointers have the same alignment as functions.

And I take it that's not 1? Can I query that value from Miri somehow, i.e., is it available the way things like pointer_size are?

On ARM, function pointers additionally store the ARM/Thumb mode in the low bit, so function pointers effectively become unaligned.

I see. So e.g. the function might have to be 4-aligned but we can still see pointers that are just 1-aligned because the bits are used for Stuff (TM). Makes sense.

And yes checking that function pointers are aligned seems like something Miri can check for. Not sure yet how the alignment of functions themselves would come in though.

@nikic
Copy link
Contributor Author

nikic commented Jul 11, 2019

FWIW that perf run looks excellent to me, so especially if you've mitigated the biggest regression (at least to some extent even if not the fullest) I'd be down for updating!

I've opened a new PR for the update at #62592.

I've also submitted https://reviews.llvm.org/D64523 as a proper fix for the SLP vectorizer perf issue, but we can pull that in later. I looked at some of the other regressions as well, but wasn't really able to identify any specific source of slow-down.

@nikic
Copy link
Contributor Author

nikic commented Jul 11, 2019

On most architectures function pointers have the same alignment as functions.

And I take it that's not 1? Can I query that value from Miri somehow, i.e., is it available the way things like pointer_size are?

It's not right now, but should be simple to expose it, just one more case in the data layout parsing. Right now there aren't any targets that actually specify a non-trivial pointer alignment though.

@RalfJung
Copy link
Member

Right now there aren't any targets that actually specify a non-trivial pointer alignment though.

Great, then right now there is nothing to do for Miri. :) Thanks!

@nikic nikic mentioned this pull request Jul 14, 2019
bors added a commit that referenced this pull request Jul 15, 2019
Update to LLVM 9 trunk

Following the preparatory changes in #62474, this updates the LLVM submodule to https://github.com/rust-lang/llvm-project/tree/rustc/9.0-2019-07-12 and:

 * Changes the LLVM Rust bindings to account for the new SubtargetSubTypeKV.
 * Adjusts a codegen test for the new form of the byval attribute that takes a type.
 * Disables LLDB on builders.
 * Moves registration of additional passes (in particular sanitizers) to the end of the module pass manager.

r? @alexcrichton
bors added a commit that referenced this pull request Jul 15, 2019
Update to LLVM 9 trunk

Following the preparatory changes in #62474, this updates the LLVM submodule to https://github.com/rust-lang/llvm-project/tree/rustc/9.0-2019-07-12 and:

 * Changes the LLVM Rust bindings to account for the new SubtargetSubTypeKV.
 * Adjusts a codegen test for the new form of the byval attribute that takes a type.
 * Makes a PGO codegen test more liberal with regard to order and linkage.
 * Builds InstrProfilingPlatformWindows.c as part of libprofiler_builtins.
 * Moves registration of additional passes (in particular sanitizers) to the end of the module pass manager.
 * Disables LLDB on builders.

r? @alexcrichton
bors added a commit that referenced this pull request Jul 16, 2019
Update to LLVM 9 trunk

Following the preparatory changes in #62474, this updates the LLVM submodule to https://github.com/rust-lang/llvm-project/tree/rustc/9.0-2019-07-12 and:

 * Changes the LLVM Rust bindings to account for the new SubtargetSubTypeKV.
 * Adjusts a codegen test for the new form of the byval attribute that takes a type.
 * Makes a PGO codegen test more liberal with regard to order and linkage.
 * Builds InstrProfilingPlatformWindows.c as part of libprofiler_builtins.
 * Moves registration of additional passes (in particular sanitizers) to the end of the module pass manager.
 * Disables LLDB on builders.

r? @alexcrichton
bors added a commit that referenced this pull request Jul 16, 2019
Update to LLVM 9 trunk

Following the preparatory changes in #62474, this updates the LLVM submodule to https://github.com/rust-lang/llvm-project/tree/rustc/9.0-2019-07-12 and:

 * Changes the LLVM Rust bindings to account for the new SubtargetSubTypeKV.
 * Adjusts a codegen test for the new form of the byval attribute that takes a type.
 * Makes a PGO codegen test more liberal with regard to order and linkage.
 * Builds InstrProfilingPlatformWindows.c as part of libprofiler_builtins.
 * Moves registration of additional passes (in particular sanitizers) to the end of the module pass manager.
 * Disables LLDB on builders.

r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants