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

incr.comp.: Turn translation-related attributes into a query. #47320

Closed
michaelwoerister opened this issue Jan 10, 2018 · 11 comments
Closed

incr.comp.: Turn translation-related attributes into a query. #47320

michaelwoerister opened this issue Jan 10, 2018 · 11 comments
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times.

Comments

@michaelwoerister
Copy link
Member

There are a few attributes, like #[inline], #[cold], and #[target_feature], that would profit from being turned into a query:

  • directly accessing the ast::Attribute will lead to a dependency edge to HIR which is brittle because of spans,
  • at least #[inline] is searched for a few times, so caching might be good.

The attributes in question are listed in trans::attributes::from_fn_attrs():

pub fn from_fn_attrs(ccx: &CrateContext, attrs: &[ast::Attribute], llfn: ValueRef) {
use syntax::attr::*;
inline(llfn, find_inline_attr(Some(ccx.sess().diagnostic()), attrs));
set_frame_pointer_elimination(ccx, llfn);
set_probestack(ccx, llfn);
let mut target_features = vec![];
for attr in attrs {
if attr.check_name("target_feature") {
if let Some(val) = attr.value_str() {
for feat in val.as_str().split(",").map(|f| f.trim()) {
if !feat.is_empty() && !feat.contains('\0') {
target_features.push(feat.to_string());
}
}
}
} else if attr.check_name("cold") {
Attribute::Cold.apply_llfn(Function, llfn);
} else if attr.check_name("naked") {
naked(llfn, true);
} else if attr.check_name("allocator") {
Attribute::NoAlias.apply_llfn(
llvm::AttributePlace::ReturnValue, llfn);
} else if attr.check_name("unwind") {
unwind(llfn, true);
} else if attr.check_name("rustc_allocator_nounwind") {
unwind(llfn, false);
}
}
if !target_features.is_empty() {
let val = CString::new(target_features.join(",")).unwrap();
llvm::AddFunctionAttrStringValue(
llfn, llvm::AttributePlace::Function,
cstr("target-features\0"), &val);
}
}

A query could return a TransFnAttrs value that looks like:

struct TransFnAttrs {
    inline: InlineAttr, // from syntax::attr
    flags: TransFnAttrFlags,
    target_features: Symbol,
}

bitflags! {
    pub struct TransFnAttrFlags: u8 {
        const COLD                      = 0b0000_0001;
        const ALLOCATOR                 = 0b0000_0010;
        const UNWIND                    = 0b0000_0100;
        const RUSTC_ALLOCATOR_NOUNWIND  = 0b0000_1000;
        const NAKED                     = 0b0001_0000;
    }
}

The syntax::attr::{find_inline_attr, requests_inline} should be replaced by this new query then.

The ultimate goal of this refactoring is to reduce the number of false positives during incr. comp. cache invalidation by providing a "firewall" between HIR and the compile_codegen_unit query.

@michaelwoerister michaelwoerister added C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. A-incr-comp Area: Incremental compilation labels Jan 10, 2018
@michaelwoerister
Copy link
Member Author

TransFnAttrFlags should also contain the #[rustc_std_internal_symbol] attribute used here:

let std_internal = attr::contains_name(&tcx.get_attrs(sym_def_id),
"rustc_std_internal_symbol");

It should also subsume the contains_extern_indicator and export_name queries.

@michaelwoerister
Copy link
Member Author

#[linkage] should also be included as an Option<rustc::mir::mono::Linkage> field.

@alexcrichton
Copy link
Member

FWIW I started doing this for target_feature in #47223, although that'd probably just be used for the final implementation of this!

@michaelwoerister
Copy link
Member Author

#47223 looks like a step in the right direction. The important thing is to extract things from attributes into queries so they can be checked for changes separately. However, since many of these things occur together, it would be a nice optimization to coalesce them.

@alexcrichton
Copy link
Member

Indeed!

@wesleywiser
Copy link
Member

I'm starting to work on this.

@michaelwoerister
Copy link
Member Author

Awesome, thank you @wesleywiser! Let me know if you need any further directions.

@wesleywiser
Copy link
Member

wesleywiser commented Feb 24, 2018

This comment is just for me to organize a TODO list of what needs to be done:

  • inline attribute
  • Various flags attributes
  • target_features attribute
  • Remove syntax::attr::find_inline_attr
  • Remove syntax::attr:requests_inline
  • rustc_std_internal_symbol attribute
  • Remove contains_extern_indicator query
  • Remove export_name query
  • linkage attribute

@michaelwoerister
Copy link
Member Author

Thanks for keeping at it, @wesleywiser!

@wesleywiser
Copy link
Member

Sorry it's taking so long @michaelwoerister. My changes for the inline attribute broke the stage1 compiler and it took awhile to find the issue.

@michaelwoerister
Copy link
Member Author

@wesleywiser, btw, I sent you a pm on gitter. I didn't find another way of contacting you.

wesleywiser added a commit to wesleywiser/rust that referenced this issue Mar 7, 2018
wesleywiser added a commit to wesleywiser/rust that referenced this issue Mar 7, 2018
wesleywiser added a commit to wesleywiser/rust that referenced this issue Mar 7, 2018
wesleywiser added a commit to wesleywiser/rust that referenced this issue Mar 7, 2018
wesleywiser added a commit to wesleywiser/rust that referenced this issue Mar 7, 2018
wesleywiser added a commit to wesleywiser/rust that referenced this issue Mar 7, 2018
wesleywiser added a commit to wesleywiser/rust that referenced this issue Mar 7, 2018
wesleywiser added a commit to wesleywiser/rust that referenced this issue Mar 7, 2018
wesleywiser added a commit to wesleywiser/rust that referenced this issue Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times.
Projects
None yet
Development

No branches or pull requests

3 participants