Prepare for LLVM 9 update by nikic · Pull Request #62474 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation28 Commits8 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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 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 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

/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 mentioned this pull request

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.

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

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...

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

This comment has been minimized.

Finished benchmarking try commit 82310f68ee567093e06ad767cc0d3cb6127457ba, comparison URL.

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.

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.

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.

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.

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.

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.

The accumulator is now respected for unordered reductions.

nikic changed the title~~[WIP] Update to LLVM 9~~ Prepare for LLVM 9 update

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.

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.

@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!

📌 Commit ac56025 has been approved by alexcrichton

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

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.

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...

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.

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.

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.

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 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

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.