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

Missed optimization: fn type calls returning never are not noreturn #64219

Closed
gnzlbg opened this issue Sep 6, 2019 · 9 comments · Fixed by #132170
Closed

Missed optimization: fn type calls returning never are not noreturn #64219

gnzlbg opened this issue Sep 6, 2019 · 9 comments · Fixed by #132170
Labels
A-codegen Area: Code generation C-enhancement Category: An issue proposing an enhancement or a PR with one. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 6, 2019

We currently do not annotate calls to function types returning Never as noreturn, but we probably should. This is similar to #64090 .

@gnzlbg gnzlbg changed the title Missed optimizations: fn type calls returning never are not noreturn Missed optimization: fn type calls returning never are not noreturn Sep 6, 2019
@varkor
Copy link
Member

varkor commented Sep 6, 2019

Here, it is suggested that we do:

if self.fn_ty.ret.layout.abi.is_uninhabited() {
// Functions with uninhabited return values are marked `noreturn`,
// so we should make sure that we never actually do.
bx.abort();
bx.unreachable();
return;
}

Also see tests src/test/codegen/noreturn-uninhabited.rs and src/test/codegen/noreturnflag.rs.

@sanxiyn
Copy link
Member

sanxiyn commented Sep 7, 2019

Closing. Currently noreturn attribute is applied, and it is tested that we do so.

@sanxiyn sanxiyn closed this as completed Sep 7, 2019
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 7, 2019

@sanxiyn where is it applied and could you point me to a test that checks this for fn type calls ?

I haven't been able to find any tests, and for this rust code:

extern "C" { static FOO: fn() -> !; }
pub unsafe fn foo() { FOO(); }

we emit:

@FOO = external global void ()*

define void @foo() unnamed_addr #0 {
start:
  %0 = load void ()*, void ()** @FOO, align 8, !nonnull !0
  call void %0()  ;; no noreturn here
  unreachable
}

like @varkor says and not

@FOO = external global void ()*

define void @foo() unnamed_addr #0 {
start:
  %0 = load void ()*, void ()** @FOO, align 8, !nonnull !0
  call void %0() #1
  unreachable
}
attributes #1 = { noreturn }

@sanxiyn
Copy link
Member

sanxiyn commented Sep 7, 2019

Where it is applied can be easily found by grepping Attribute::NoReturn inside librustc_codegen_llvm.

@sanxiyn
Copy link
Member

sanxiyn commented Sep 7, 2019

Guess: it seems noreturn is applied for Rust functions but not applied for extern functions?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 7, 2019

So how do you explain that the LLVM-IR generated by rustc that I just showed does not contain it ?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 7, 2019

@sanxiyn this issue is about function types, like fn() -> ! in let x: fn() -> ! = ...; x();.

@sanxiyn
Copy link
Member

sanxiyn commented Sep 7, 2019

Oh.

@sanxiyn sanxiyn reopened this Sep 7, 2019
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 7, 2019

@varkor that comment says that functions returning uninhabited types are marked noreturn, but we can't mark function pointers noreturn (they don't support LLVM attributes). We can add attributes to the call site of the function pointer, e.g., as shown above, but since we are emitting an unreachable instruction right afterwards, then maybe it isn't necessary to do that ? If the function pointer call were to return, then unreachable would be reached, and the behavior would be undefined.

@jonas-schievink jonas-schievink added A-codegen Area: Code generation C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 10, 2019
@workingjubilee workingjubilee added the C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such label Oct 8, 2023
@jieyouxu jieyouxu added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Oct 26, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Oct 27, 2024
…ieyouxu

Add a Few Codegen Tests

Closes rust-lang#86109
Closes rust-lang#64219

Those issues somehow got fixed over time.

So, this PR adds a couple of codegen tests to ensure we don't regress in the future.
@bors bors closed this as completed in a513e0e Nov 2, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 2, 2024
Rollup merge of rust-lang#132170 - veera-sivarajan:codegen-tests, r=jieyouxu

Add a Few Codegen Tests

Closes rust-lang#86109
Closes rust-lang#64219

Those issues somehow got fixed over time.

So, this PR adds a couple of codegen tests to ensure we don't regress in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-enhancement Category: An issue proposing an enhancement or a PR with one. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code. 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