Make codegen tests compatible with extra inlining#78967
Make codegen tests compatible with extra inlining#78967bors merged 3 commits intorust-lang:masterfrom
Conversation
The memory allocation in vec might panic in the case of capacity overflow. Move the allocation outside the function to fix the test.
rust-highfive
added
the
S-waiting-on-review
tmiasko
force-pushed
the
codegen-tests
branch
from
596889c to
d54ea4f
Compare
cuviper
left a comment
cuviper
left a comment
There was a problem hiding this comment.
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.
Check that LLVM understands that
Iterpointer 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.
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.
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.
The fact that pointer is not-null is expressed through llvm.assume that follows, see next check.
There was a problem hiding this comment.
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 |
bors
added
S-waiting-on-bors
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
5 participants
No description provided.