Separate Builder methods from tcx

[ZuseZ4]

As part of the autodiff upstreaming we noticed, that it would be nice to have various builder methods available without the TypeContext, which prevents the normal CodegenCx to be passed around between threads.
We introduce a SimpleCx which just owns the llvm module and llvm context, to encapsulate them.
The previous CodegenCx now implements deref and forwards access to the llvm module or context to it's SimpleCx sub-struct. This gives us a bit more flexibility, because now we can pass (or construct) the SimpleCx in locations where we don't have enough information to construct a CodegenCx, or are not able to pass it around due to the tcx lifetimes (and it not implementing send/sync).

This also introduces an SBuilder, similar to the SimpleCx. The SBuilder uses a SimpleCx, whereas the existing Builder uses the larger CodegenCx. I will push updates to make implementations generic (where possible) to be implemented once and work for either of the two. I'll also clean up the leftover code.

call is a bit tricky, because it requires a tcx, I probably need to duplicate it after all.

Tracking:

• Tracking Issue for autodiff #124509

[rustbot]

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[ZuseZ4]

r? oli-obk

lol, I was too slow.

[rustbot]

Could not assign reviewer from: oli-obk.
User(s) oli-obk are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

[rustbot]

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

[oli-obk]

That's not quite right, is it? Probably not even right for the normal builder

[ZuseZ4]

yep, I'll adjust it.

[oli-obk]

So my hope was that we could change the regular builder to be

pub(crate) struct Builder<'a, 'll, CX: Deref<Target = SimpleCx<'ll>>> {
    pub llbuilder: &'ll mut llvm::Builder<'ll>,
    pub cx: CX,
}

(may need to use Borrow instead of Deref and impl it for the context types, because we wanna store &'a CX, but then CX won't impl Deref)

Once that works, you can have impls for either impl<CX> Builder<CX> or impl Builder<CodegenCx> depending on what is needed, and the methods would be available for both.

This transition can be simplified a lot by renaming Builder to GenericBuilder and having a type alias named Builder.

[ZuseZ4]

Partly rubberducking here.

I implemented Deref like this:

impl<'ll> Borrow<SimpleCx<'ll>> for CodegenCx<'ll, '_> {
    fn borrow(&self) -> &SimpleCx<'ll> {
        &self.scx
    }
}

One thing I don't fully understand yet, when do I add/use the 'tcx lifetime which is required by CodegenCx, but not SimpleCx? E.g. the drop implementation below works

pub(crate) struct GenericBuilder<'a, 'll, CX: Borrow<SimpleCx<'ll>>> {
    pub llbuilder: &'ll mut llvm::Builder<'ll>,
    pub cx: &'a CX,
}

type Builder<'a, 'll, 'tcx> = GenericBuilder<'a, 'll, CodegenCx<'ll, 'tcx>>;

impl<'a, 'll, CX: Borrow<SimpleCx<'ll>>> Drop for GenericBuilder<'a, 'll, CX> {
    fn drop(&mut self) {
        unsafe {
            llvm::LLVMDisposeBuilder(&mut *(self.llbuilder as *mut _));
        }
    }
}

However, trying to use the type alias you mentioned fails, e.g. here:

impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
    type CodegenCx = CodegenCx<'ll, 'tcx>;

    fn build(cx: &'a CodegenCx<'ll, 'tcx>, llbb: &'ll BasicBlock) -> Self {
        let bx = Builder::with_cx(cx);
        unsafe {
            llvm::LLVMPositionBuilderAtEnd(bx.llbuilder, llbb);
        }
        bx
    }
...

gives the following error:

Diagnostics:                                                                                                                                                        
 1. the trait bound `GenericBuilder<'a, 'll, context::CodegenCx<'ll, 'tcx>>: rustc_codegen_ssa::traits::AsmBuilderMethods<'tcx>` is not satisfied                    
    the trait `rustc_codegen_ssa::traits::AsmBuilderMethods<'tcx>` is not implemented for `GenericBuilder<'a, 'll, context::CodegenCx<'ll, 'tcx>>` [E0277]           
 2. the trait bound `GenericBuilder<'a, 'll, context::CodegenCx<'ll, 'tcx>>: rustc_codegen_ssa::traits::IntrinsicCallBuilderMethods<'tcx>` is not satisfied          
    the trait `rustc_codegen_ssa::traits::IntrinsicCallBuilderMethods<'tcx>` is not implemented for `GenericBuilder<'a, 'll, context::CodegenCx<'ll, 'tcx>>` [E0277] 
 3. the trait bound `GenericBuilder<'a, 'll, context::CodegenCx<'ll, 'tcx>>: rustc_codegen_ssa::traits::AbiBuilderMethods<'tcx>` is not satisfied                    
    the trait `rustc_codegen_ssa::traits::AbiBuilderMethods<'tcx>` is not implemented for `GenericBuilder<'a, 'll, context::CodegenCx<'ll, 'tcx>>` [E0277]           
 4. the trait bound `GenericBuilder<'a, 'll, context::CodegenCx<'ll, 'tcx>>: rustc_codegen_ssa::traits::ArgAbiBuilderMethods<'tcx>` is not satisfied                 
    the trait `rustc_codegen_ssa::traits::ArgAbiBuilderMethods<'tcx>` is not implemented for `GenericBuilder<'a, 'll, context::CodegenCx<'ll, 'tcx>>` [E0277]        
 5. the trait bound `GenericBuilder<'a, 'll, context::CodegenCx<'ll, 'tcx>>: rustc_codegen_ssa::traits::DebugInfoBuilderMethods` is not satisfied                    
    the trait `rustc_codegen_ssa::traits::DebugInfoBuilderMethods` is not implemented for `GenericBuilder<'a, 'll, context::CodegenCx<'ll, 'tcx>>` [E0277]           
 6. the trait bound `GenericBuilder<'a, 'll, context::CodegenCx<'ll, 'tcx>>: CoverageInfoBuilderMethods<'tcx>` is not satisfied                                      
    the trait `CoverageInfoBuilderMethods<'tcx>` is not implemented for `GenericBuilder<'a, 'll, context::CodegenCx<'ll, 'tcx>>` [E0277]   

on the positive side, the SimpleCx seems to work flawless:

type SBuilder<'a, 'll> = GenericBuilder<'a, 'll, SimpleCx<'ll>>;

allowed me to delete the drop implementation for SBuilder, since now we can use the one from above for the GenericBuilder.

[ZuseZ4]

The first attempt I pushed gives these 4 errors:

error[E0621]: explicit lifetime required in the type of `bx`
   --> compiler/rustc_codegen_llvm/src/intrinsic.rs:720:9
    |
713 |     bx: &mut Builder<'_, 'll, '_>,
    |         ------------------------- help: add explicit lifetime `'ll` to the type of `bx`: `&mut GenericBuilder<'_, 'll, context::CodegenCx<'ll, 'll>>`
...
720 |         bx.set_personality_fn(bx.eh_personality());
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lifetime `'ll` required

error[E0621]: explicit lifetime required in the type of `bx`
   --> compiler/rustc_codegen_llvm/src/intrinsic.rs:867:9
    |
860 |     bx: &mut Builder<'_, 'll, '_>,
    |         ------------------------- help: add explicit lifetime `'ll` to the type of `bx`: `&mut GenericBuilder<'_, 'll, context::CodegenCx<'ll, 'll>>`
...
867 |         bx.set_personality_fn(bx.eh_personality());
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lifetime `'ll` required

error[E0621]: explicit lifetime required in the type of `bx`
   --> compiler/rustc_codegen_llvm/src/intrinsic.rs:963:20
    |
944 |     bx: &mut Builder<'_, 'll, '_>,
    |         ------------------------- help: add explicit lifetime `'ll` to the type of `bx`: `&mut GenericBuilder<'_, 'll, context::CodegenCx<'ll, 'll>>`
...
963 |         let then = bx.append_sibling_block("then");
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lifetime `'ll` required

error[E0621]: explicit lifetime required in the type of `bx`
    --> compiler/rustc_codegen_llvm/src/intrinsic.rs:1027:20
     |
1003 |     bx: &mut Builder<'_, 'll, '_>,
     |         ------------------------- help: add explicit lifetime `'ll` to the type of `bx`: `&mut GenericBuilder<'_, 'll, context::CodegenCx<'ll, 'll>>`
...
1027 |         let then = bx.append_sibling_block("then");
     |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lifetime `'ll` required

I seem to be able to solve it so far by replacing all anonymous lifetimes with 'a or 'tcx and updating 'll to 'll: 'tcx.
That in turn causes various other lifetime errors, but as far as I got I seem to be able to fix all with the these three changes.
Since the tcx has the llvm context/module as subfields, requiring that the llvm pieces live as long as the whole tcx seems reasonable? But of course updating everything isn't great, please lmk if I'm missing something.

[ZuseZ4]

This seems like a never-ending story, everytime I fix one file, it shows me errors in new files.
Since I'm not even sure if this is the right thing to do, can you please confirm that, before I spend more time on it?
Also, there is one file where I'll probably need some other solution:
compiler/rustc_codegen_llvm/src/base.rs gives these errors (also see log):

error[E0597]: `llvm_module` does not live long enough
   --> compiler/rustc_codegen_llvm/src/base.rs:86:47
    |
76  |     fn module_codegen<'ll: 'tcx, 'tcx>(tcx: TyCtxt<'tcx>, cgu_name: Symbol) -> ModuleCodegen<ModuleLlvm> {
    |                       --- lifetime `'ll` defined here
...
84  |         let llvm_module = ModuleLlvm::new(tcx, cgu_name.as_str());
    |             ----------- binding `llvm_module` declared here
85  |         {
86  |             let cx = CodegenCx::new(tcx, cgu, &llvm_module);
    |                      -------------------------^^^^^^^^^^^^-
    |                      |                        |
    |                      |                        borrowed value does not live long enough
    |                      argument requires that `llvm_module` is borrowed for `'ll`
...
141 |     }
    |     - `llvm_module` dropped here while still borrowed

error[E0505]: cannot move out of `llvm_module` because it is borrowed
   --> compiler/rustc_codegen_llvm/src/base.rs:138:26
    |
76  |     fn module_codegen<'ll: 'tcx, 'tcx>(tcx: TyCtxt<'tcx>, cgu_name: Symbol) -> ModuleCodegen<ModuleLlvm> {
    |                       --- lifetime `'ll` defined here
...
84  |         let llvm_module = ModuleLlvm::new(tcx, cgu_name.as_str());
    |             ----------- binding `llvm_module` declared here
85  |         {
86  |             let cx = CodegenCx::new(tcx, cgu, &llvm_module);
    |                      --------------------------------------
    |                      |                        |
    |                      |                        borrow of `llvm_module` occurs here
    |                      argument requires that `llvm_module` is borrowed for `'ll`
...
138 |             module_llvm: llvm_module,
    |                          ^^^^^^^^^^^ move out of `llvm_module` occurs here

[ZuseZ4]

I moved over all the cheap wins, methods which only call llvm:: functions.
I will duplicate the two call methods by having one call the other and move things a bit around to reduce the diff, but other than that it should be good (?) and it compiles.
Let me know if you want any extra changes.

[ZuseZ4]

@oli-obk So there's an issue with 3 functions (e.g. ret_void) from the BuilderMethods trait which I duplicated, to add generic versions. In the future, we probably want to implement the whole trait over the GenericBuilder instead of the Builder, but for now I don't see a good way around it. Shall we temporarily allow that?

[oli-obk]

Yes. That seems totally fine. Thanks for doing all this btw!

[oli-obk]

Signing off for the night

@bors delegate+

r=me with commented out code removed

[bors]

✌️ @ZuseZ4, you can now approve this pull request!

If @oli-obk told you to "r=me" after making some further change, please make that change, then do @bors r=@oli-obk

[ZuseZ4]

@bors r=@oli-obk

[bors]

📌 Commit 386c233 has been approved by oli-obk

It is now in the queue for this repository.