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

[EXPERIMENT] Figure out which commit caused regression in parallel alt builder #94604

Closed

Conversation

b-naber
Copy link
Contributor

@b-naber b-naber commented Mar 4, 2022

#93800 caused the build times for dist-x86_64-msvc-alt to significantly increase. Trying to figure which of the changes in that PR is responsible for the regression.

master:

Screenshot 2022-03-04 at 14 22 34

https://github.com/rust-lang/rust/runs/5421496388?check_suite_focus=true

with these changes:

diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs
index 2126487da02..5642d01de5d 100644
--- a/compiler/rustc_middle/src/mir/mod.rs
+++ b/compiler/rustc_middle/src/mir/mod.rs
@@ -2533,7 +2533,7 @@ pub enum ConstantKind<'tcx> {

 impl<'tcx> Constant<'tcx> {
     pub fn check_static_ptr(&self, tcx: TyCtxt<'_>) -> Option<DefId> {
-        match self.literal.const_for_ty()?.val().try_to_scalar() {
+        match self.literal.try_to_scalar() {
             Some(Scalar::Ptr(ptr, _size)) => match tcx.global_alloc(ptr.provenance) {
                 GlobalAlloc::Static(def_id) => {
                     assert!(!tcx.is_thread_local_static(def_id));

Screenshot 2022-03-04 at 15 53 28

https://github.com/rust-lang/rust/runs/5422857349?check_suite_focus=true

with these changes:

diff --git a/compiler/rustc_middle/src/thir.rs b/compiler/rustc_middle/src/thir.rs
index 40dce281c82..a4e2a7c3e91 100644
--- a/compiler/rustc_middle/src/thir.rs
+++ b/compiler/rustc_middle/src/thir.rs
@@ -17,6 +17,7 @@
 use rustc_index::vec::IndexVec;
 use rustc_middle::infer::canonical::Canonical;
 use rustc_middle::middle::region;
+use rustc_middle::mir::interpret::AllocId;
 use rustc_middle::mir::{
     BinOp, BorrowKind, FakeReadCause, Field, Mutability, UnOp, UserTypeProjection,
 };
@@ -419,7 +420,8 @@ pub enum ExprKind<'tcx> {
     /// This is only distinguished from `Literal` so that we can register some
     /// info for diagnostics.
     StaticRef {
-        literal: Const<'tcx>,
+        alloc_id: AllocId,
+        ty: Ty<'tcx>,
         def_id: DefId,
     },
     /// Inline assembly, i.e. `asm!()`.
diff --git a/compiler/rustc_middle/src/thir/visit.rs b/compiler/rustc_middle/src/thir/visit.rs
index 95489ac3ab2..b3e2cb132a2 100644
--- a/compiler/rustc_middle/src/thir/visit.rs
+++ b/compiler/rustc_middle/src/thir/visit.rs
@@ -123,7 +123,7 @@ pub fn walk_expr<'a, 'tcx: 'a, V: Visitor<'a, 'tcx>>(visitor
: &mut V, expr: &Exp
         }
         Closure { closure_id: _, substs: _, upvars: _, movability: _, fake_reads: _ } => {}
         Literal { literal, user_ty: _, const_id: _ } => visitor.visit_const(literal),
-        StaticRef { literal, def_id: _ } => visitor.visit_const(literal),
+        StaticRef { .. } => {}
         InlineAsm { ref operands, template: _, options: _, line_spans: _ } => {
             for op in &**operands {
                 use InlineAsmOperand::*;
diff --git a/compiler/rustc_mir_build/src/build/expr/as_constant.rs b/compiler/rustc_mir_build/src/build/expr/as_constant.rs
index 79ac09d523d..0c0b0f2bd05 100644
--- a/compiler/rustc_mir_build/src/build/expr/as_constant.rs
+++ b/compiler/rustc_mir_build/src/build/expr/as_constant.rs
@@ -1,6 +1,7 @@
 //! See docs in build/expr/mod.rs
 
 use crate::build::Builder;
+use rustc_middle::mir::interpret::{ConstValue, Scalar};
 use rustc_middle::mir::*;
 use rustc_middle::thir::*;
 use rustc_middle::ty::CanonicalUserTypeAnnotation;
@@ -26,8 +27,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 assert_eq!(literal.ty(), ty);
                 Constant { span, user_ty, literal: literal.into() }
             }
-            ExprKind::StaticRef { literal, .. } => {
-                Constant { span, user_ty: None, literal: literal.into() }
+            ExprKind::StaticRef { alloc_id, ty, .. } => {
+                let const_val =
+                    ConstValue::Scalar(Scalar::from_pointer(alloc_id.into(), &this.tcx));
+                let literal = ConstantKind::Val(const_val, ty);
+
+                Constant { span, user_ty: None, literal }
             }
             ExprKind::ConstBlock { value } => {
                 Constant { span: span, user_ty: None, literal: value.into() }
diff --git a/compiler/rustc_mir_build/src/thir/cx/expr.rs b/compiler/rustc_mir_build/src/thir/cx/expr.rs
index 651edc827c3..5a7e1d88dd0 100644
--- a/compiler/rustc_mir_build/src/thir/cx/expr.rs
+++ b/compiler/rustc_mir_build/src/thir/cx/expr.rs
@@ -8,7 +8,6 @@
 use rustc_middle::hir::place::PlaceBase as HirPlaceBase;
 use rustc_middle::hir::place::ProjectionKind as HirProjectionKind;
 use rustc_middle::middle::region;
-use rustc_middle::mir::interpret::Scalar;
 use rustc_middle::mir::{BinOp, BorrowKind, Field, UnOp};
 use rustc_middle::thir::*;
 use rustc_middle::ty::adjustment::{
@@ -941,15 +940,8 @@ fn convert_path_expr(&mut self, expr: &'tcx hir::Expr<'tcx>, res: Res) -> ExprKi
                 let kind = if self.tcx.is_thread_local_static(id) {
                     ExprKind::ThreadLocalRef(id)
                 } else {
-                    let ptr = self.tcx.create_static_alloc(id);
-                    ExprKind::StaticRef {
-                        literal: ty::Const::from_scalar(
-                            self.tcx,
-                            Scalar::from_pointer(ptr.into(), &self.tcx),
-                            ty,
-                        ),
-                        def_id: id,
-                    }
+                    let alloc_id = self.tcx.create_static_alloc(id);
+                    ExprKind::StaticRef { alloc_id, ty, def_id: id }
                 };
                 ExprKind::Deref {
                     arg: self.thir.exprs.push(Expr { ty, temp_lifetime, span: expr.span, kind }),

Screenshot 2022-03-04 at 17 57 17

https://github.com/rust-lang/rust/runs/5424277709?check_suite_focus=true

with these changes:

04cd0b9

Screenshot 2022-03-04 at 19 33 44

https://github.com/rust-lang/rust/runs/5425665818?check_suite_focus=true

r? @ghost

@rust-log-analyzer

This comment has been minimized.

@b-naber b-naber force-pushed the static-initializers-experiment branch from b07ce2a to 0b6ec3a Compare March 4, 2022 11:29
@rust-log-analyzer

This comment has been minimized.

@b-naber
Copy link
Contributor Author

b-naber commented Mar 4, 2022

master:

Screenshot 2022-03-04 at 14 22 34

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 4, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@b-naber b-naber force-pushed the static-initializers-experiment branch from 87c8554 to 04cd0b9 Compare March 4, 2022 17:10
@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Mar 4, 2022

⌛ Trying commit f67c905 with merge 5c77b41a5150105b3790dfdef1ed612c996e31fa...

@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-msvc-alt failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Updating files:  99% (31879/32201)
Updating files:  99% (31992/32201)
Updating files: 100% (32201/32201)
Updating files: 100% (32201/32201), done.
Note: switching to 'refs/remotes/pull/94604/merge'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

---
  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 66877f56 Merge f67c9054a90a4ba3a96b1a8f264abc298ac1b0be into b4bf56cd66ca83e908fd43bde4c627f94b2a8a9f
[command]"C:\Program Files\Git\bin\git.exe" log -1 --format='%H'
'66877f56f5006b487a2a2257df1621a9f74bcd36'
'66877f56f5006b487a2a2257df1621a9f74bcd36'
##[group]Run echo "[CI_PR_NUMBER=$num]"
echo "[CI_PR_NUMBER=$num]"
env:
  CI_JOB_NAME: dist-x86_64-msvc-alt
  SCCACHE_BUCKET: rust-lang-ci-sccache2
  TOOLSTATE_REPO: https://github.com/rust-lang-nursery/rust-toolstate

@bors
Copy link
Contributor

bors commented Mar 4, 2022

☀️ Try build successful - checks-actions
Build commit: 5c77b41a5150105b3790dfdef1ed612c996e31fa (5c77b41a5150105b3790dfdef1ed612c996e31fa)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants