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 tests for identity matching results #100692

Closed
wants to merge 1 commit into from

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Aug 17, 2022

This is an attempt to write codegen tests to ensure that returning an identity of a Result does not have unnecessary overhead (#37939).

This is my first codegen test, so take it easy on me.

Btw, I noticed that for Result<u32, u32> it generates less optimal code (https://rust.godbolt.org/z/YEcjTboYK). Seems like some aggregate ABI issue.

r? @scottmcm

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 17, 2022
@scottmcm
Copy link
Member

Doh, we raced on this -- I opened #100693 two minutes after you did this one 😆

@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 17, 2022

Aaah! :D Well, let's keep your PR, you know much more about this. At least I learned how to write a codegen test :)

@Kobzol Kobzol closed this Aug 17, 2022
@scottmcm
Copy link
Member

Looks like you did a pretty good job! Always good to have more people who can help out with pinning or E-needs-test things!

The two review notes I'd have to help you in future:

  • Some of the builders run on system LLVM that's not our up-to-date in-tree LLVM, thus why you'll see things like // no-system-llvm in a bunch of them. So these tests -- where it's an LLVM change that made them work -- need to have some sort of attribute or they'll pass CI but break in bors on those other LLVMs.
  • I strongly suggest using CHECK-LABEL to help make sure your other CHECKs match where you think they're going to match, rather than potentially matching way later in the file and giving a confusing error. See https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-label-directive.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-13 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
failures:

---- [codegen] src/test/codegen/try_identity.rs stdout ----

error: verification with 'FileCheck' failed
status: exit status: 1
command: "/usr/lib/llvm-13/bin/FileCheck" "--input-file" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/try_identity/try_identity.ll" "/checkout/src/test/codegen/try_identity.rs" "--allow-unused-prefixes" "--check-prefixes" "CHECK,NONMSVC"
stdout: none
--- stderr -------------------------------
/checkout/src/test/codegen/try_identity.rs:12:16: error: CHECK-NEXT: expected string not found in input
// CHECK-NEXT: ret i64 %0
               ^
/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/try_identity/try_identity.ll:8:7: note: scanning from here
      ^
      ^
/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/try_identity/try_identity.ll:14:2: note: possible intended match here
 ret i64 %.sroa.07.0.insert.insert

Input file: /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/try_identity/try_identity.ll
Check file: /checkout/src/test/codegen/try_identity.rs


-dump-input=help explains the following input dump.
Input was:
<<<<<<
<<<<<<
           1: ; ModuleID = 'try_identity.55f9fcc1-cgu.0' 
           2: source_filename = "try_identity.55f9fcc1-cgu.0" 
           3: target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" 
           4: target triple = "x86_64-unknown-linux-gnu" 
           5:  
           6: ; Function Attrs: mustprogress nofree norecurse nosync nounwind nonlazybind readnone uwtable willreturn 
           7: define i64 @result_nop_match(i64 %0) unnamed_addr #0 { 
           8: start: 
next:12'0           X error: no match found
           9:  %_2 = and i64 %0, 4294967295 
next:12'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          10:  %switch = icmp ne i64 %_2, 0 
next:12'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          11:  %. = zext i1 %switch to i64 
next:12'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          12:  %.sroa.4.0.extract.shift = and i64 %0, -4294967296 
next:12'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          13:  %.sroa.07.0.insert.insert = or i64 %.sroa.4.0.extract.shift, %. 
next:12'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          14:  ret i64 %.sroa.07.0.insert.insert 
next:12'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
next:12'1      ?                                  possible intended match
          15: } 
next:12'0     ~~
          16:  
next:12'0     ~
          17: ; Function Attrs: mustprogress nofree norecurse nosync nounwind nonlazybind readnone uwtable willreturn 
next:12'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          18: define i64 @result_nop_try_block(i64 %0) unnamed_addr #0 { 
next:12'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          19: start: 
next:12'0     ~~~~~~~
           .
           .
>>>>>>
------------------------------------------

@scottmcm
Copy link
Member

Ah, thank you, PR build, for running with LLVM 13 and conveniently illustrating my point 🙃

@scottmcm
Copy link
Member

Btw, I noticed that for Result<u32, u32> it generates less optimal code (https://rust.godbolt.org/z/YEcjTboYK). Seems like some aggregate ABI issue.

Oh, that's really interesting! Since Result is repr(Rust), I know of no reason why Result<i32,i32>/Result<i32,u32>/Result<u32,i32>/Result<u32,u32> shouldn't just all use the same ABI -- though I also don't know much about ABI.

Can you file an issue about it? Definitely seems weird enough to be worth one, at least.

(Though it's nice that it's still a nop, just a two-register nop instead of a one-register nop.)

@Kobzol Kobzol deleted the try-op-codegen-test branch August 17, 2022 20:09
@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 17, 2022

Thanks a lot for the hints! I filed #100698.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants