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

Add codegen test for 112169 #124087

Closed
wants to merge 1 commit into from

Conversation

SadiinsoSnowfall
Copy link

@SadiinsoSnowfall SadiinsoSnowfall commented Apr 17, 2024

Add codegen test for #112169.
The test passes but it's my first time using FileCheck, don't hesitate to tell me if this can be improved ^^

Fixes #112169

@rustbot
Copy link
Collaborator

rustbot commented Apr 17, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 17, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 17, 2024
tests/codegen/issues/issue-112169.rs Outdated Show resolved Hide resolved
tests/codegen/issues/issue-112169.rs Outdated Show resolved Hide resolved
tests/codegen/issues/issue-112169.rs Outdated Show resolved Hide resolved
@clubby789 clubby789 changed the title Add codegen test for #112169 Add codegen test for 112169 Apr 18, 2024
@Mark-Simulacrum
Copy link
Member

r=me with commits squashed. Thanks!

@SadiinsoSnowfall
Copy link
Author

@bors r=@Mark-Simulacrum

Commits squashed 🫡

@bors
Copy link
Contributor

bors commented Apr 21, 2024

@SadiinsoSnowfall: 🔑 Insufficient privileges: Not in reviewers

@clubby789
Copy link
Contributor

@bors r=@Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Apr 21, 2024

📌 Commit 9c77de1 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 21, 2024
…lacrum

Add codegen test for 112169

Add codegen test for rust-lang#112169.
The test passes but it's my first time using FileCheck, don't hesitate to tell me if this can be improved ^^
@bors
Copy link
Contributor

bors commented Apr 21, 2024

⌛ Testing commit 9c77de1 with merge 5c6b635...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 21, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 21, 2024
@SadiinsoSnowfall
Copy link
Author

Well, that's... surprising

@clubby789
Copy link
Contributor

Looks like LLVM doesn't make any optimizations in LoopVectorization in WASM for this, whereas it does in other architectures, preventing later improvements.

@SadiinsoSnowfall
Copy link
Author

Is this by design ? If so, should I add something like @ ignore-wasm in the codegen test ?

@clubby789
Copy link
Contributor

//@ ignore-wasm FIXME: LLVM does not vectorize loops on WASM so this doesn't get optimized Should be okay

@nikic
Copy link
Contributor

nikic commented Apr 23, 2024

Hm, this makes me think that this issue isn't really fixed. We get it now as a combination of vectorization and unrolling, but that just pushes the constant up. If you replace 102 with say 1002, the issue still exists.

As far as I can tell, this is a phase ordering problem now. We successfully peel the first iteration now and bring the loop into an analyzable form, but this happens after induction variable simplification has already run, so we fail to simplify the loop further.

@apiraino
Copy link
Contributor

I'm curious about the status of this PR. Can anyone with some context share and what is needed to move it forward. I'll set it to waiting on author because I'm not completely sure it's ready for review

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 17, 2024
@apiraino apiraino added the A-codegen Area: Code generation label Jun 17, 2024
@SadiinsoSnowfall
Copy link
Author

SadiinsoSnowfall commented Jun 17, 2024

The related issue hasn't been solved yet, so there's no need to add a codegen test yet I guess :/
Should I delete this PR ?

@nikic
Copy link
Contributor

nikic commented Jun 17, 2024

Yeah, I think we can close this PR for now ... the situation has improved, but the issue isn't fully fixed yet.

@apiraino apiraino removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird codegen / constant folding with iterators of length > 101
9 participants