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 test for issue 106269 #124299

Merged
merged 1 commit into from
Apr 30, 2024
Merged

Add test for issue 106269 #124299

merged 1 commit into from
Apr 30, 2024

Conversation

clubby789
Copy link
Contributor

Closes #106269

Made this an assembly test as the LLVM codegen is still quite verbose and doesn't really indicate the behaviour we want

@rustbot
Copy link
Collaborator

rustbot commented Apr 23, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2024
@Mark-Simulacrum
Copy link
Member

r? @nikic since #106269 (comment) sounded to me like there might be a better way to test here (e.g., detecting the memcmp somehow?)

@clubby789
Copy link
Contributor Author

The IR looks like this - it's only during codegen that it can be turned into a single comparison

define noundef zeroext i1 @eq1(ptr noalias nocapture noundef readonly align 1 dereferenceable(4) %s1, ptr noalias nocapture noundef readonly align 1 dereferenceable(4) %s2) unnamed_addr {
start:
  %_4 = load i8, ptr %s1, align 1
  %_5 = load i8, ptr %s2, align 1
  %_3 = icmp eq i8 %_4, %_5
  br i1 %_3, label %bb1, label %bb8

bb1:
  %0 = getelementptr inbounds i8, ptr %s1, i64 1
  %_7 = load i8, ptr %0, align 1
  %1 = getelementptr inbounds i8, ptr %s2, i64 1
  %_8 = load i8, ptr %1, align 1
  %_6 = icmp eq i8 %_7, %_8
  br i1 %_6, label %bb2, label %bb8

bb2:
  %2 = getelementptr inbounds i8, ptr %s1, i64 2
  %_10 = load i8, ptr %2, align 1
  %3 = getelementptr inbounds i8, ptr %s2, i64 2
  %_11 = load i8, ptr %3, align 1
  %_9 = icmp eq i8 %_10, %_11
  br i1 %_9, label %bb3, label %bb8

bb3:
  %4 = getelementptr inbounds i8, ptr %s1, i64 3
  %_12 = load i8, ptr %4, align 1
  %5 = getelementptr inbounds i8, ptr %s2, i64 3
  %_13 = load i8, ptr %5, align 1
  %6 = icmp eq i8 %_12, %_13
  br label %bb8

bb8:
  %_0.sroa.0.0 = phi i1 [ %6, %bb3 ], [ false, %bb2 ], [ false, %bb1 ], [ false, %start ]
  ret i1 %_0.sroa.0.0
}

@nikic
Copy link
Contributor

nikic commented Apr 29, 2024

@bors r+ rollup=iffy

Assembly tests are the best we can do for this right now. After llvm/llvm-project#77370 we'd be able to use IR tests for these.

@bors
Copy link
Contributor

bors commented Apr 29, 2024

📌 Commit 11b0e9a has been approved by nikic

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 29, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2024
Add test for issue 106269

Closes rust-lang#106269

Made this an assembly test as the LLVM codegen is still quite verbose and doesn't really indicate the behaviour we want
@bors
Copy link
Contributor

bors commented Apr 29, 2024

⌛ Testing commit 11b0e9a with merge c016833...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 29, 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 29, 2024
@clubby789
Copy link
Contributor Author

Looks like there's always a

        push    rbp
        mov     rbp, rsp
        ....
        pop     rbp

for code on Darwin

@bors r=nikic

@bors
Copy link
Contributor

bors commented Apr 30, 2024

📌 Commit 7032c92 has been approved by nikic

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 30, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2024
Rollup of 4 pull requests

Successful merges:

 - rust-lang#124280 (Port repr128-dwarf run-make test to rmake)
 - rust-lang#124299 (Add test for issue 106269)
 - rust-lang#124553 (Write `git-commit-{sha,info}` for Cargo in source tarballs)
 - rust-lang#124561 (Add `normalize()` in run-make `Diff` type)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4b6c191 into rust-lang:master Apr 30, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Apr 30, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2024
Rollup merge of rust-lang#124299 - clubby789:106269-test, r=nikic

Add test for issue 106269

Closes rust-lang#106269

Made this an assembly test as the LLVM codegen is still quite verbose and doesn't really indicate the behaviour we want
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suboptimal eq compilation on structs compared to equivalent C++ code
6 participants