rustc_codegen_llvm: traitification of LLVM-specific CodegenCx and Builder methods #55627

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

Merged

bors merged 73 commits into rust-lang:master from sunfishcode:cg-llvm-gen

Nov 17, 2018

Member

sunfishcode commented Nov 2, 2018 •

edited by eddyb

Loading

This PR is the continuation of #54012 and earlier PRs, in the grand plan of #45274 to allow for multiple codegen backends.

High-level summary: interpose a set of traits between Rust's codegen logic and the LLVM APIs, allowing another backend to implement the traits and share most of the codegen logic. These traits are currently somewhat LLVM-specific, but once this refactoring is in place, they can evolve to be more general.

See this README for a writeup on the current trait organization.

rust-highfive assigned pnkfelix

Contributor

rust-highfive commented Nov 2, 2018

(rust_highfive has picked a reviewer for you, use r? to override)

rust-highfive added the S-waiting-on-review label

sunfishcode mentioned this pull request

rustc_codegen_llvm: traitification of LLVM-specific CodegenCx and Builder methods #54012

Closed

This comment has been minimized.

Sign in to view

sunfishcode force-pushed the cg-llvm-gen branch from 6538a47 to cffad3f Compare

November 2, 2018 22:30

Member Author

sunfishcode commented Nov 2, 2018

rust-highfive assigned eddyb and unassigned pnkfelix

This comment has been minimized.

Sign in to view

Contributor

denismerigoux commented Nov 3, 2018

Thanks for fixing the bug ! It was a silly mistake from my part...

eddyb mentioned this pull request

rustc_target: pass contexts by reference, not value. #55665

Merged

bors added a commit that referenced this pull request


          Auto merge of #55665 - eddyb:by-ref-layout-of, r=oli-obk

e6c5cf9

rustc_target: pass contexts by reference, not value.

`LayoutOf` now takes `&self` instead of `self`, and so does every method generic over a context that implements `LayoutOf` and/or other traits, like `HasDataLayout`, `HasTyCtxt`, etc.

Originally using by-value `Copy` types was relevant because `TyCtxt` was one of those types, but now `TyCtxt::layout_of` is separate from `LayoutOf`, and `TyCtxt` is not an often used layout context.

Passing these context by reference is a lot nicer for miri, which has `self: &mut EvalContext`, and needed `f(&self)` (that is, creating `&&mut EvalContext` references) for layout purposes.
Now, the `&mut EvalContext` can be passed to a function expecting `&C`, directly.

This should help with #54012 / #55627 (to not need `where &'a T::Cx: LayoutOf` bounds).

r? @nikomatsakis or @oli-obk or @nagisa cc @sunfishcode

eddyb suggested changes

View reviewed changes

Member

eddyb left a comment

First batch of comments (my browser hates me already!)

src/librustc_codegen_llvm/abi.rs Outdated

Member

eddyb Nov 4, 2018

dst should probably be on its own line.

src/librustc_codegen_llvm/abi.rs Outdated

Member

eddyb Nov 4, 2018

I think this is a LLVM-ism, does Cranelift support getting at the function params from a builder? What I'd do instead is also pass a slice of values, alongside the index.

src/librustc_codegen_llvm/abi.rs Outdated

Member

eddyb Nov 4, 2018

Maybe have a crate-wide type-alias so that we can continue to write CodegenCx<'ll, '_> instead of CodegenCx<'ll, '_, &'ll Value> in rustc_codegen_llvm?

src/librustc_codegen_llvm/abi.rs Outdated

Member

eddyb Nov 4, 2018

Hmm, I think it might be better if instead of the "apply attrs" LLVM-ism, the call/invoke methods of Builder took an FnType and internally called the LLVM-only apply_attrs_callsite.

src/librustc_codegen_llvm/abi.rs Outdated

Member

eddyb Nov 4, 2018

Do any of these depend on anything LLVM-specific? IMO these could be moved into rustc_codegen_ssa or even rustc itself!

src/librustc_codegen_llvm/builder.rs Outdated

Member

eddyb Nov 4, 2018

For this to be a safe Rust function, btw, *const c_char needs to be replaced with &CStr.

src/librustc_codegen_llvm/builder.rs Outdated

Member

eddyb Nov 4, 2018

This makes me think From should be implemented instead. Then, the LLVM-specific AtomicOrdering do not need to be imported, removing rustc_codegen_ssa::common:: noise in the signature and we can just do order.into() and whatnot.

src/librustc_codegen_llvm/builder.rs Outdated

Member

eddyb Nov 4, 2018

This probably needs to be generalized to storage_{live,dead} (which is what the MIR contains).

src/librustc_codegen_llvm/builder.rs Outdated

Member

eddyb Nov 4, 2018

This is unsafe! The trait method should be marked as such. Maybe we can find a way to avoid doing it at all.

src/librustc_codegen_llvm/builder.rs Outdated

Member

eddyb Nov 4, 2018

Applying attributes via the return value is a LLVM-ism, we should clean it up.

eddyb suggested changes

View reviewed changes

Member

eddyb left a comment

Second round of comments, got through rustc_codegen_llvm and rustc_codegen_ssa::back, things seems pretty good so far!

src/librustc_codegen_llvm/callee.rs Outdated

Member

eddyb Nov 4, 2018

Any reason this can't be a method in an extension trait on TyLayout still?
That is, I'd prefer to write cx.layout_of(fn_ptr_ty).codegen_type(cx).
OTOH, another thing we can do is remove the & there.
TyLayout is a "cheap" type, a pair of references IIRC, by design!

src/librustc_codegen_llvm/common.rs Outdated

Member

eddyb Nov 4, 2018

This should probably be moved to the consts.rs file or something.

src/librustc_codegen_llvm/common.rs Outdated

Member

eddyb Nov 4, 2018

I'd maybe call this codegen_scalar or, better, const_scalar.

src/librustc_codegen_llvm/consts.rs Outdated

Member

eddyb Nov 4, 2018

This seems like would require this method to be marked as unsafe (as references to old_g might still be around, outside of the LLVM module itself).

src/librustc_codegen_llvm/context.rs Outdated

Member

eddyb Nov 4, 2018

Oh, this is local struct. You can do V = &'ll Value here!

src/librustc_codegen_llvm/type_.rs Outdated

Member

eddyb Nov 4, 2018

These should be inverted - that is, implemented on TyLayout, in rustc_codegen_ssa. I guess you still need this trait, hmm.

src/librustc_codegen_ssa/README.md Outdated

Member

eddyb Nov 4, 2018

I think you could have Value in a trait that's only associated types, and is implemented for builders to point to their backend's associated types.

src/librustc_codegen_ssa/back/write.rs Outdated

Member

eddyb Nov 4, 2018

This reminds me the name Codegen in the context of the "back" module, is suboptimal.
Including in "optimize_and_codegen". I think we could call this step "emitting machine code/a binary", so maybe optimize_and_emit? OptimizeAndEmitContext is not great, but more descriptive.

cc @michaelwoerister @rkruppe @alexcrichton

Also, : should be : .

src/librustc_codegen_ssa/back/write.rs Outdated

Member

eddyb Nov 4, 2018

Not sure what to do about all of these LLVM-specific options.

Member

eddyb Nov 4, 2018

Maybe rename bitcode to ir, to generalize? or llvm_bc, to make it clear it's LLVM-specific.

src/librustc_codegen_ssa/back/write.rs Outdated

Member

eddyb Nov 4, 2018

Maybe someone should search for llvm (case-insensitive) in rustc_codegen_ssa.

Member

eddyb commented Nov 4, 2018

There's a general issue, that rustc_codegen_ssa should perhaps not have all of that back stuff in it. Would it make sense to move it to rustc_codegen_link or something similar?
Maybe rustc_post_codegen. Name ideas welcome!

eddyb suggested changes

View reviewed changes

Member

eddyb left a comment

Third batch, and that's all the comments for now! Let me know if I can help resolve everything.

src/librustc_codegen_ssa/base.rs Outdated

Member

eddyb Nov 5, 2018

Outdated comment - could even be removed, base ended up as a dumping ground (and common similarly), they should be properly redistributed around IMO.

Member

eddyb Nov 5, 2018

But only the comment is a concern for now.

src/librustc_codegen_ssa/base.rs Outdated

Member

eddyb Nov 5, 2018

So, &'a Cx : LayoutOf + HasTyCtxt bounds should not longer be needed, since my PR landed.

Also, we either need to 1. avoid putting bounds on structs 2. enable the implied bounds feature to avoid writing the bounds anywhere else. cc @nikomatsakis

Member

eddyb Nov 5, 2018

Also, are any of these "stats" useful anymore? It feels like any new infrastructure should be designed with MIR in mind...

src/librustc_codegen_ssa/base.rs Outdated

Member

eddyb Nov 5, 2018

IMO Real in the type name should be changed to Float. I have no idea why LLVM chose that (wrong) name in the first place.

src/librustc_codegen_ssa/base.rs Outdated

Member

eddyb Nov 5, 2018

As I mentioned earlier, I think it would be cleaner if builders implemented the same trait holding associated types, so this was just Bx::Value.

Also, I'm seeing 'll in this crate - that shouldn't be needed!
After all, we're abstracting over types like &'ll Value - so the parametrization should only exist in rustc_codegen_llvm.

src/librustc_codegen_ssa/base.rs Outdated

Member

eddyb Nov 5, 2018

I'm impressed all of this entrypoint stuff could be reused!

src/librustc_codegen_utils/lib.rs Outdated

Member

eddyb Nov 5, 2018

Why is this needed? It should, at all, be placed on individual items!

src/librustc_codegen_ssa/mir/block.rs Outdated

Member

eddyb Nov 5, 2018

Could we, instead, make Bx an associated type on Cx? That would get rid of some of the explicit parametrization.

src/librustc_codegen_ssa/mir/block.rs Outdated

Member

eddyb Nov 5, 2018

I think this should be on an extension trait on TyCtxt in rustc_codegen_ssa.

src/librustc_codegen_ssa/mir/operand.rs Outdated

Member

eddyb Nov 5, 2018

These packing methods should be relegated to the backend.
AFAIK, Cranelift uses multiple returns instead of packing two values into one.

src/librustc_codegen_ssa/mir/rvalue.rs Outdated

Member

eddyb Nov 5, 2018

This should be a builder method.

denismerigoux force-pushed the cg-llvm-gen branch from cffad3f to 1764f2c Compare

November 5, 2018 17:16

Contributor

rust-highfive commented Nov 5, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:082fe952:start=1541438357623593167,finish=1541438360154799359,duration=2531206192
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---

[00:03:34] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:35] tidy error: /checkout/src/librustc_codegen_ssa/callee.rs:31: line longer than 100 chars
[00:03:36] some tidy checks failed
[00:03:36] 
[00:03:36] 
[00:03:36] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:36] 
[00:03:36] 
[00:03:36] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:36] Build completed unsuccessfully in 0:00:47
[00:03:36] Build completed unsuccessfully in 0:00:47
[00:03:36] make: *** [tidy] Error 1
[00:03:36] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0a861f50
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:05cb73b0:start=1541438587433574914,finish=1541438587437863110,duration=4288196
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0d0af16f
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:06f8a32c
travis_time:start:06f8a32c
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:158fe32a
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Collaborator

bors commented Nov 5, 2018

☔ The latest upstream changes (presumably #55410) made this pull request unmergeable. Please resolve the merge conflicts.

denismerigoux force-pushed the cg-llvm-gen branch from 1764f2c to 09235bb Compare

November 6, 2018 10:17

Contributor

rust-highfive commented Nov 6, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:381eeeba:start=1541499549548654605,finish=1541499551832758207,duration=2284103602
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---

[00:04:16] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:16] tidy error: /checkout/src/librustc_codegen_ssa/callee.rs:31: line longer than 100 chars
[00:04:18] some tidy checks failed
[00:04:18] 
[00:04:18] 
[00:04:18] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:18] 
[00:04:18] 
[00:04:18] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:18] Build completed unsuccessfully in 0:00:51
[00:04:18] Build completed unsuccessfully in 0:00:51
[00:04:18] Makefile:79: recipe for target 'tidy' failed
[00:04:18] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:05016710
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:1b67f44c:start=1541499820877229265,finish=1541499820881839828,duration=4610563
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0246a805
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:06e9a240
travis_time:start:06e9a240
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:04d4854c
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

eddyb mentioned this pull request

Move cg_llvm::rpath and parts of cg_llvm::back::link to rustc_codegen_utils #55725

Closed

Collaborator

bors commented Nov 7, 2018

☔ The latest upstream changes (presumably #55746) made this pull request unmergeable. Please resolve the merge conflicts.

Member

Mark-Simulacrum commented Nov 15, 2018

@sunfishcode Can you take a look at fixing the merge conflicts and resolving the CI failure here?

Mark-Simulacrum added S-waiting-on-author and removed S-waiting-on-review labels

Member

eddyb commented Nov 15, 2018

@Mark-Simulacrum I'm rebasing it and getting close to finishing.

irinagpopa and others added 2 commits

November 16, 2018 14:10


          rustc_codegen_llvm: begin generalizing over backend values.

5a9e6b8


          Work around to fix issue rust-lang#53912

c76fc3d

Collaborator

bors commented Nov 17, 2018

⌛ Testing commit 756f84d with merge c5b7d98c594251886e5f58bf7197a8aa59053ad3...

Collaborator

bors commented Nov 17, 2018

💔 Test failed - status-appveyor

bors added S-waiting-on-review and removed S-waiting-on-bors labels

Member

eddyb commented Nov 17, 2018

@alexcrichton @retep998 Is this spurious, or a real failure?

@bors retry (just in case)

bors added S-waiting-on-bors and removed S-waiting-on-review labels

Collaborator

bors commented Nov 17, 2018

⌛ Testing commit 756f84d with merge f6e9485...

bors added a commit that referenced this pull request


          Auto merge of #55627 - sunfishcode:cg-llvm-gen, r=nikomatsakis

f6e9485

rustc_codegen_llvm: traitification of LLVM-specific CodegenCx and Builder methods

This PR is the continuation of #54012 and earlier PRs, in the grand plan of #45274 to allow for multiple codegen backends.

High-level summary: interpose a set of traits between Rust's codegen logic and the LLVM APIs, allowing another backend to implement the traits and share most of the codegen logic. These traits are currently somewhat LLVM-specific, but once this refactoring is in place, they can evolve to be more general.

See [this README](https://github.com/rust-lang/rust/blob/756f84d7cef90b7364ae88ca707e59670dde4c92/src/librustc_codegen_ssa/README.md) for a writeup on the current trait organization.

Collaborator

bors commented Nov 17, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing f6e9485 to master...

bors merged commit 756f84d into rust-lang:master

This was referenced Nov 17, 2018

Upgrade LLVM to trunk, still version 8 #55835

Merged

codegen_llvm_back: improve allocations #55871

Merged

Decouple proc_macro from the rest of the compiler. #49219

Merged

[WIP] Take 2: change how MIR represents Place #54426

Closed

Add Weak.ptr_eq #55987

Merged

Make librustc_codegen_llvm aware of LLVM address spaces. #51576

Closed

libcore: Add VaList and variadic arg handling intrinsics #49878

Merged

Use pinning for generators to make trait safe #55704

Merged

This was referenced Nov 18, 2018

Add rustc_codegen_ssa to sysroot #56048

Merged

Use rustc_codegen_ssa rust-lang/rustc_codegen_cranelift#170

Open

This was referenced Nov 20, 2018

Refactorings for rustc_codegen_ssa #56108

Open

Refactor librustc_codegen_llvm for multiple codegen backends. #45274

Open

kennytm added a commit to kennytm/rust that referenced this pull request


          Rollup merge of rust-lang#56048 - bjorn3:cg_ssa_sysroot, r=eddyb

1b707f7

Add rustc_codegen_ssa to sysroot

Outside of rustc you are currently unable to use it.

r? @nikomatsakis (because you r+'ed rust-lang#55627)

sunfishcode deleted the cg-llvm-gen branch

November 5, 2021 19:43

ehuss mentioned this pull request

Enable tests on rustc_codegen_ssa #114431

Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request


          Rollup merge of rust-lang#114431 - ehuss:ssa-test, r=est31

Enable tests on rustc_codegen_ssa

This enables unittests in rustc_codegen_ssa. There are some tests, primarily in [`back/rpath/tests.rs`](https://github.com/rust-lang/rust/blob/HEAD/compiler/rustc_codegen_ssa/src/back/rpath/tests.rs) that haven't ever been running since the unittests are disabled. From what I can tell, this was just a consequence of how things evolved. When testing was initially added in rust-lang#33282, `librustc_trans` had test=false because it didn't have any tests. `rustc_codegen_ssa` eventually split off from that (rust-lang#55627), and the rpath module eventually got merged in too (from `librustc_back` where it used to live). That migration didn't enable the tests.

This also includes some fluent diagnostic tests, though I'm not sure what exactly they are testing.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request


          Rollup merge of rust-lang#114431 - ehuss:ssa-test, r=est31

353e268

Enable tests on rustc_codegen_ssa

This enables unittests in rustc_codegen_ssa. There are some tests, primarily in [`back/rpath/tests.rs`](https://github.com/rust-lang/rust/blob/HEAD/compiler/rustc_codegen_ssa/src/back/rpath/tests.rs) that haven't ever been running since the unittests are disabled. From what I can tell, this was just a consequence of how things evolved. When testing was initially added in rust-lang#33282, `librustc_trans` had test=false because it didn't have any tests. `rustc_codegen_ssa` eventually split off from that (rust-lang#55627), and the rpath module eventually got merged in too (from `librustc_back` where it used to live). That migration didn't enable the tests.

This also includes some fluent diagnostic tests, though I'm not sure what exactly they are testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors