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

Worsened debug build codegen in beta #68855

Closed
rodrimati1992 opened this issue Feb 5, 2020 · 7 comments · Fixed by #70259
Closed

Worsened debug build codegen in beta #68855

rodrimati1992 opened this issue Feb 5, 2020 · 7 comments · Fixed by #70259
Assignees
Labels
A-associated-items Area: Associated items such as associated types and consts. A-const-eval Area: constant evaluation (mir interpretation) I-slow Issue: Problems and improvements with respect to performance of generated code. P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented Feb 5, 2020

The assembly generated in debug builds (-C opt-level=0) worsened for this code that uses an associated constant from a trait.This seems to be caused by the fact that the function is generic,even though the associated constant doesn't depend on the generic parameters of the function.

The code:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=83dfb31158166b5ed7a92ca5dd7bb610

trait NeedsDrop:Sized{
    const NEEDS:bool=std::mem::needs_drop::<Self>();
}

impl<This> NeedsDrop for This{}

pub fn hello<T>(){
    if <bool>::NEEDS {
        panic!()
    }
}

pub fn hi(){
    hello::<()>();
    hello::<Vec<()>>();
}

The entirety of the generated assembly for stable 1.41.0 in a debug build:

playground::hello:
    retq

playground::hello:
    retq

playground::hi:
    pushq   %rax
    callq   *playground::hello@GOTPCREL(%rip)
    callq   *playground::hello@GOTPCREL(%rip)
    popq    %rax
    retq

The assembly generated for the same functions by rustc 1.42.0-beta.2 (2020-02-04 3d2613e) in a debug build:

playground::hello:
    pushq   %rax
    xorl    %eax, %eax
    testb   $1, %al
    jne .LBB45_2
    jmp .LBB45_1

.LBB45_1:
    popq    %rax
    retq

.LBB45_2:
    leaq    .Lanon.257b63aea292a17f4811b89c35bea688.2(%rip), %rdi
    leaq    .Lanon.257b63aea292a17f4811b89c35bea688.4(%rip), %rdx
    movq    std::panicking::begin_panic@GOTPCREL(%rip), %rax
    movl    $14, %esi
    callq   *%rax
    ud2

playground::hello:
    pushq   %rax
    xorl    %eax, %eax
    testb   $1, %al
    jne .LBB46_2
    jmp .LBB46_1

.LBB46_1:
    popq    %rax
    retq

.LBB46_2:
    leaq    .Lanon.257b63aea292a17f4811b89c35bea688.2(%rip), %rdi
    leaq    .Lanon.257b63aea292a17f4811b89c35bea688.4(%rip), %rdx
    movq    std::panicking::begin_panic@GOTPCREL(%rip), %rax
    movl    $14, %esi
    callq   *%rax
    ud2

playground::hi:
    pushq   %rax
    callq   *playground::hello@GOTPCREL(%rip)
    callq   *playground::hello@GOTPCREL(%rip)
    popq    %rax
    retq

@jonas-schievink jonas-schievink added I-slow Issue: Problems and improvements with respect to performance of generated code. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 5, 2020
@pnkfelix
Copy link
Member

pnkfelix commented Feb 6, 2020

@rodrimati1992 out of curiosity, what prompted you to investigate this specific code sequence?

(I'm trying to evaluate the severity of this code-quality regression, and it might help for me to get some idea of how wide-spread the problem might be. There are obviously a couple different features interacting in the example you have provided...)

@pnkfelix
Copy link
Member

pnkfelix commented Feb 6, 2020

triage: leaving unprioritized. Nominating for discussion at today's T-compiler triage meeting.

@rodrimati1992
Copy link
Contributor Author

@pnkfelix
I was experimenting to figure out the best design for a feature I want to add to structural eventually,being efficient in debug builds is a plus(but not a hard requirement for me).
The reason I reported this is that if it optimizes fine in non-generic functions,it should also do the same optimization in generic functions(so long as the constant doesn't depend on generic parameters).

@eddyb
Copy link
Member

eddyb commented Feb 8, 2020

    xorl    %eax, %eax
    testb   $1, %al

How does LLVM even generate such a code sequence? Well...:

br i1 false, label %bb2, label %bb1, !dbg !945

Okay so I guess LLVM doesn't even simplify the CFG when optimizations are off?


Anyway, the MIR differs. This is a constant folding regression.
cc @rust-lang/wg-mir-opt

@oli-obk
Copy link
Contributor

oli-obk commented Feb 10, 2020

I'm guessing this is caused by #67631

We're in a generic context and resolve has to be conservative so it doesn't accidentally try to inline a default value if it could get a different value in an impl. In this situation that can't happen, but I don't know how to coax resolve to know this.

@eddyb
Copy link
Member

eddyb commented Feb 10, 2020

We're in a generic context and resolve has to be conservative so it doesn't accidentally try to inline a default value if it could get a different value in an impl. In this situation that can't happen, but I don't know how to coax resolve to know this.

The fix was already implemented though! Instance::resolve already returns None in the problematic cases. Users of Instance::resolve don't need to special-case anything anymore.

Nor do we ever need to use UserFacing just because something might be polymorphic, because the fix is in Instance::resolve already, as it has been in traits::project for associated types.

I left comments here: #67662 (comment)

@pnkfelix
Copy link
Member

T-compiler triage: P-medium. Leaving nomination tag on until we actually assign this to someone.

@pnkfelix pnkfelix added P-medium Medium priority A-associated-items Area: Associated items such as associated types and consts. A-const-eval Area: constant evaluation (mir interpretation) labels Feb 20, 2020
Centril added a commit to Centril/rust that referenced this issue Mar 24, 2020
Use Reveal::All in MIR optimizations

Resolves some code review feedback in rust-lang#67662.

Fixes rust-lang#68855

r? @eddyb
Centril added a commit to Centril/rust that referenced this issue Mar 24, 2020
Use Reveal::All in MIR optimizations

Resolves some code review feedback in rust-lang#67662.

Fixes rust-lang#68855

r? @eddyb
@bors bors closed this as completed in 0d5b83d Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-associated-items Area: Associated items such as associated types and consts. A-const-eval Area: constant evaluation (mir interpretation) I-slow Issue: Problems and improvements with respect to performance of generated code. P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants