-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Make codegen tests compatible with extra inlining #78967
Conversation
The memory allocation in vec might panic in the case of capacity overflow. Move the allocation outside the function to fix the test.
596889c
to
d54ea4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why we should be modifying tests without any corresponding compiler changes, but it does look like issue-37945.rs
is problematic...
// CHECK-NOT: icmp eq float* {{.*}}, null | ||
// CHECK-LABEL: @is_empty_1( | ||
// CHECK-NEXT: start: | ||
// CHECK-NEXT: [[A:%.*]] = icmp ne i32* %xs.1, null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check that LLVM understands that
Iter
pointer is not null.
If we have icmp ne ..., null
, it seems like we're failing the thing we meant to test, no?
I don't think we should try to compare exactly what LLVM IR comes out, but perhaps something like:
/// CHECK-NOT: icmp {{.*}}, null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have
icmp ne ..., null
, it seems like we're failing the thing we meant to test, no?
It was fixed upstream by canonicalizing to assume.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "it was fixed upstream", do you mean that will be in LLVM 12? Is there something we could cherry-pick?
My point is the test meant to check that LLVM knows it's not null, so we had a CHECK-NOT: icmp eq ..., null
to that effect. Your change reveals that while we were passing that test, we're getting icmp ne ..., null
instead, which seems just as bad. So it seems like a real bug with or without your change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that pointer is not-null is expressed through llvm.assume
that follows, see next check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, ok, I see what you mean...
From my perspective those are bugs in tests. Fixes are mostly for my own convenience to be able to run tests with extra inlining. If you don't want to land some / any of those that seems fair. |
OK, if CI is happy, I'm happy. @bors r+ |
📌 Commit d54ea4f has been approved by |
Make codegen tests compatible with extra inlining
Rollup of 9 pull requests Successful merges: - rust-lang#77939 (Ensure that the source code display is working with DOS backline) - rust-lang#78138 (Upgrade dlmalloc to version 0.2) - rust-lang#78967 (Make codegen tests compatible with extra inlining) - rust-lang#79027 (Limit storage duration of inlined always live locals) - rust-lang#79077 (document that __rust_alloc is also magic to our LLVM fork) - rust-lang#79088 (clarify `span_label` documentation) - rust-lang#79097 (Code block invalid html tag lint) - rust-lang#79105 (std: Fix test `symlink_hard_link` on Windows) - rust-lang#79107 (build-manifest: strip newline from rustc version) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
No description provided.