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

Update LLVM submodule #107879

Merged
merged 3 commits into from
Mar 2, 2023
Merged

Update LLVM submodule #107879

merged 3 commits into from
Mar 2, 2023

Conversation

icedrocket
Copy link
Contributor

@icedrocket icedrocket commented Feb 10, 2023

Fixes #105626

@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2023

⚠️ Warning ⚠️

  • These commits modify submodules.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 10, 2023
@cuviper
Copy link
Member

cuviper commented Feb 11, 2023

Can you add a regression test for this? Probably a ui run-pass test with min-system-llvm-version: 16.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 11, 2023
@icedrocket
Copy link
Contributor Author

icedrocket commented Feb 19, 2023

// int -> f32
assert_eq::<f32>(127i8 as f32, 127.0);
assert_eq::<f32>(2147483647i32 as f32, 2147483648.0);
assert_eq::<f32>((-2147483648i32) as f32, -2147483648.0);
assert_eq::<f32>(1234567890i32 as f32, /*0x1.26580cp+30*/ f32::from_bits(0x4e932c06));
assert_eq::<f32>(16777217i32 as f32, 16777216.0);
assert_eq::<f32>((-16777217i32) as f32, -16777216.0);
assert_eq::<f32>(16777219i32 as f32, 16777220.0);
assert_eq::<f32>((-16777219i32) as f32, -16777220.0);
assert_eq::<f32>(
0x7fffff4000000001i64 as f32,
/*0x1.fffffep+62*/ f32::from_bits(0x5effffff),
);
assert_eq::<f32>(
0x8000004000000001u64 as i64 as f32,
/*-0x1.fffffep+62*/ f32::from_bits(0xdeffffff),
);
assert_eq::<f32>(
0x0020000020000001i64 as f32,
/*0x1.000002p+53*/ f32::from_bits(0x5a000001),
);
assert_eq::<f32>(
0xffdfffffdfffffffu64 as i64 as f32,
/*-0x1.000002p+53*/ f32::from_bits(0xda000001),
);
// FIXME emscripten does not support i128
#[cfg(not(target_os = "emscripten"))]
{
assert_eq::<f32>(i128::MIN as f32, -170141183460469231731687303715884105728.0f32);
assert_eq::<f32>(u128::MAX as f32, f32::INFINITY); // saturation
}

It seems that this part doesn't check unsigned to float casting for edge cases. We need to add unsigned to float casting checks and modify assert_eq to check both static and runtime casting.

@cuviper
Copy link
Member

cuviper commented Feb 20, 2023

It seems that this part doesn't check unsigned to float casting for edge cases. We need to add unsigned to float casting checks and modify assert_eq to check both static and runtime casting.

I think that's fine to add separately, now that there's a targeted test here.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 20, 2023

📌 Commit 1c97cde6f11dab37312faef4a2460339115ae3c8 has been approved by cuviper

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 Feb 20, 2023
@bors
Copy link
Contributor

bors commented Feb 21, 2023

⌛ Testing commit 1c97cde6f11dab37312faef4a2460339115ae3c8 with merge 97abed1fd6352f808f6309387c82f36fcc9959e3...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 21, 2023

💔 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 Feb 21, 2023
@cuviper
Copy link
Member

cuviper commented Feb 21, 2023

Those values failed equality on i586-unknown-linux-gnu, but print identically -- that might be more x87 excess precision at work on i586. Maybe we should just limit this test to only-i686-pc-windows-msvc?

@cuviper
Copy link
Member

cuviper commented Feb 22, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Feb 22, 2023

📌 Commit 1e4ba39b026dff0378b3612d12da1a9f2e02f128 has been approved by cuviper

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 Feb 22, 2023
@icedrocket
Copy link
Contributor Author

I force-pushed to parent commit by accident. Now ready for review.

@cuviper
Copy link
Member

cuviper commented Feb 22, 2023

I don't think black_box is a reliable way to avoid x87 issues. What was wrong with your sse2 version of the test? This regression test is specifically for a particular target platform; it's ok if we don't run that test elsewhere.

@icedrocket
Copy link
Contributor Author

I don't think black_box is a reliable way to avoid x87 issues. What was wrong with your sse2 version of the test? This regression test is specifically for a particular target platform; it's ok if we don't run that test elsewhere.

The u64 to f32 conversion is currently implemented using x87 even when SSE2 is enabled. If in the future the compiler changes to use SSE2 for that conversion, it will be difficult to know if there is a problem with the x87 implementation as the test will not fail as long as SSE2 is enabled.

@icedrocket
Copy link
Contributor Author

But I think it's okay for us to just use the SSE2 version for now. As long as SSE2 is enabled, we can generate code that works fine on x86. I will contribute to solving the x87 issue on the LLVM side later.

@cuviper
Copy link
Member

cuviper commented Feb 23, 2023

The LLVM fix is specific to Windows, and i686-pc-windows-msvc does have SSE2 enabled by default. I'm not sure what else you're hoping to fix in LLVM, but I think we're good here.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 23, 2023

📌 Commit 313f04f has been approved by cuviper

It is now in the queue for this repository.

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Feb 25, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 26, 2023
@bors
Copy link
Contributor

bors commented Feb 28, 2023

⌛ Testing commit 313f04f with merge 2f5bc1f0b395c73a727be74e7f1b277a36a386c3...

@bors
Copy link
Contributor

bors commented Feb 28, 2023

💔 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 Feb 28, 2023
@cuviper
Copy link
Member

cuviper commented Feb 28, 2023

(x86_64-gnu-llvm-14, 1, ubuntu-20.04-xl):

---- config::tests::download_ci_llvm stdout ----
Detected LLVM as non-available: running in CI and modified LLVM in this change
Detected LLVM as non-available: running in CI and modified LLVM in this change
Detected LLVM as non-available: running in CI and modified LLVM in this change
Detected LLVM as non-available: running in CI and modified LLVM in this change
thread 'config::tests::download_ci_llvm' panicked at 'assertion failed: parse_llvm(\"build.build = \\\"x86_64-unknown-linux-gnu\\\"\")', config/tests.rs:22:5
error: test failed, to rerun pass `--lib`

@rust-lang/bootstrap -- this should be expected! We're updating LLVM, but not all CI builders use that.
(though I'm not certain I understand the failure mode here...)

@jyn514
Copy link
Member

jyn514 commented Feb 28, 2023

@cuviper oops, this might be the first time LLVM's been updated since #107234 - I think we should skip this test altogether if LLVM was updated in the PR. Maybe take the code in

if CiEnv::is_ci() {
// We assume we have access to git, so it's okay to unconditionally pass
// `true` here.
let llvm_sha = detect_llvm_sha(config, true);
let head_sha = output(config.git().arg("rev-parse").arg("HEAD"));
let head_sha = head_sha.trim();
if llvm_sha == head_sha {
eprintln!(
"Detected LLVM as non-available: running in CI and modified LLVM in this change"
);
return false;
}
and separate it into a function?

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Feb 28, 2023
@cuviper
Copy link
Member

cuviper commented Feb 28, 2023

@jyn514 I gave that a shot, let me know what you think!

Copy link
Member

@albertlarsan68 albertlarsan68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve the last commit (91af58ac58dee5794fe54a8f9cb5556d3585688d).
Just a minor nit, if you want.

src/bootstrap/native.rs Outdated Show resolved Hide resolved
@cuviper
Copy link
Member

cuviper commented Feb 28, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Feb 28, 2023

📌 Commit 565de58 has been approved by cuviper

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 Feb 28, 2023
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 2, 2023

⌛ Testing commit 565de58 with merge 18caf88...

@bors
Copy link
Contributor

bors commented Mar 2, 2023

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing 18caf88 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 2, 2023
@bors bors merged commit 18caf88 into rust-lang:master Mar 2, 2023
@rustbot rustbot added this to the 1.69.0 milestone Mar 2, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (18caf88): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Casting the same i64 and u64 integers to f32 gives different results on i686-pc-windows-msvc target
9 participants