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

builder::do_not_inline #87

Open
antoyo opened this issue Sep 25, 2021 · 7 comments
Open

builder::do_not_inline #87

antoyo opened this issue Sep 25, 2021 · 7 comments
Assignees
Labels
libgccjit requires a change in libgccjit

Comments

@antoyo
Copy link
Contributor

antoyo commented Sep 25, 2021

No description provided.

@antoyo antoyo self-assigned this Sep 25, 2021
@antoyo antoyo added the libgccjit requires a change in libgccjit label Sep 25, 2021
@ghost
Copy link

ghost commented Sep 26, 2021

Mind if I take this? I think the best way is to add this function:

void gcc_jit_function_add_attribute(gcc_jit_function *func, const char *attr);

And apply noinline attribute to the function in question.

@antoyo
Copy link
Contributor Author

antoyo commented Sep 26, 2021

I already have a gcc branch locally and a codegen branch locally adding support for the inline attribute.
I'll push those tomorrow and then I'll tell you if there's any work to do besides actually using it in the do_not_inline method ;) .

@antoyo
Copy link
Contributor Author

antoyo commented Sep 26, 2021

I pushed a PR in gcc with early support for inlining.

The codegen part was pushed in a branch (now having an unrelated history, sorry :) ) there.

@antoyo
Copy link
Contributor Author

antoyo commented Sep 27, 2021

I've just realized that this is actually telling a function call to not be inlined, not the function itself.
Not sure how we can do that, especially with the current API where do_not_inline() is called on the value returned by the function call.

Two ideas (requiring a change in the API):

  • Create a wrapper function with noinline attribute.
  • Call via a function pointer (not sure if it's as efficient).

@ghost
Copy link

ghost commented Sep 28, 2021

I've just realized that this is actually telling a function call to not be inlined, not the function itself.

Some quick reseach suggests that it might be false:

Here's how this method defined in LLVM backend:

fn do_not_inline(&mut self, llret: &'ll Value) {
    llvm::Attribute::NoInline.apply_callsite(llvm::AttributePlace::Function, llret);
}

// impl Attribute
pub fn apply_callsite(&self, idx: AttributePlace, callsite: &Value) {
    unsafe { LLVMRustAddCallSiteAttribute(callsite, idx.as_uint(), *self) }
}


impl AttributePlace {
    pub fn as_uint(self) -> c_uint {
        match self {
            AttributePlace::ReturnValue => 0,
            AttributePlace::Argument(i) => 1 + i,
            AttributePlace::Function => !0,
        }
    }
}
extern "C" void LLVMRustAddCallSiteAttribute(LLVMValueRef Instr, unsigned Index,
                                             LLVMRustAttribute RustAttr) {
  CallBase *Call = unwrap<CallBase>(Instr);
  Attribute Attr = Attribute::get(Call->getContext(), fromRust(RustAttr));
  Call->AddAttributeAtIndex(Index, Attr);
}

The trick here is the AttributePlace index. The name of the enum variant makes me think it's applied to the function itself, but I'm not familiar with LLVM, so you'll need to consult a third party to verify, sorry.


Anyway, I don't think a pointer call is a good idea. It may be inlined anyway in some circumstances, and it wastes a register.

The wrapper option looks better to me. You can even attribute it with flatten so the real fn will be inlined inside it.

@ghost
Copy link

ghost commented Sep 29, 2021

This method is only called for one thing: to prevent exponential inlining of drop glue in landing pads, see rust-lang/rust#41696 and rust-lang/rust#41920

Even though this looks LLVM specific, I suspect GCC may suffer from this issue as well. Anyway, it'll be a while before we get to unwind support, so for now I'll get back to crashing always_inline.

@antoyo
Copy link
Contributor Author

antoyo commented Sep 29, 2021

To be honest, I might start working on landing pads soon, as it seems most UI tests that fail are because they're not implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libgccjit requires a change in libgccjit
Projects
None yet
Development

No branches or pull requests

1 participant