Skip to content

datetime tests fail in release mode since 1.73 on powerpc64le #116582

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
cuviper opened this issue Oct 9, 2023 · 7 comments · Fixed by #116840
Closed

datetime tests fail in release mode since 1.73 on powerpc64le #116582

cuviper opened this issue Oct 9, 2023 · 7 comments · Fixed by #116840
Assignees
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-PowerPC Target: PowerPC processors P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@cuviper
Copy link
Member

cuviper commented Oct 9, 2023

The datetime tests have regressed on Fedora ppc64le since upgrading to Rust 1.73.0, and I can also reproduce this on rustup toolchains.
https://koschei.fedoraproject.org/build/16447830

I expected to see this happen: all tests pass

Instead, this happened: it passes in debug mode, but fails multiple tests in --release. For example:

$ cargo test --test instant_to_datetime --release -- the_end_of_time
[...]
     Running tests/instant_to_datetime.rs (target/release/deps/instant_to_datetime-dc863ddaa1b40632)

running 1 test
test the_end_of_time ... FAILED

failures:

---- the_end_of_time stdout ----
thread 'the_end_of_time' panicked at tests/instant_to_datetime.rs:80:5:
assertion `left == right` failed
  left: -1
 right: 7

Version it worked on

It most recently worked on: 1.72.1

Version with regression

rustc --version --verbose:

rustc 1.73.0 (cc66ad468 2023-10-03)
binary: rustc
commit-hash: cc66ad468955717ab92600c770da8c1601a4ff33
commit-date: 2023-10-03
host: powerpc64le-unknown-linux-gnu
release: 1.73.0
LLVM version: 17.0.2

It's no better on rustc 1.74.0-beta.1 (b5c050feb 2023-10-03) or rustc 1.75.0-nightly (bf9a1c8a1 2023-10-08).

As noted above, it also fails on Fedora rawhide's Rust 1.73.0 with LLVM 17. However, it passes on Fedora 38 with Rust 1.73.0 and LLVM 16, so it definitely seems like a codegen regression in 17.

@rustbot modify labels: +A-LLVM +T-compiler +regression-from-stable-to-stable -regression-untriaged

@cuviper cuviper added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Oct 9, 2023
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. I-prioritize Issue: Indicates that prioritization has been requested for this issue. A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed regression-untriaged Untriaged performance or correctness regression. labels Oct 9, 2023
@saethlin saethlin added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 9, 2023
@Mark-Simulacrum Mark-Simulacrum added this to the 1.73.0 milestone Oct 9, 2023
@nikic
Copy link
Contributor

nikic commented Oct 10, 2023

Doesn't happen when calling at_ms() with a zero argument instead, so probably something gets miscompiled when at_ms() is inlined into at() and specialized for 0.

@nikic
Copy link
Contributor

nikic commented Oct 10, 2023

opt-bisect points towards a backend issue:

BISECT: running pass (19571) Two-Address instruction pass on function (_ZN8datetime3cal8datetime13LocalDateTime2at17h068a3ad6fbe82580E)
BISECT: NOT running pass (19572) Machine Instruction Scheduler on function (_ZN8datetime3cal8datetime13LocalDateTime2at17h068a3ad6fbe82580E)

IR for the function looks like this: https://gist.github.com/nikic/6752e057cf4f933e592277e4a9211d6a

@nikic nikic self-assigned this Oct 10, 2023
@nikic
Copy link
Contributor

nikic commented Oct 10, 2023

The issue is already introduced during the SDAG layer by a custom combine: https://github.com/llvm/llvm-project/blob/12a4757b0fa25baac853498671a34e2f34f92b80/llvm/lib/Target/PowerPC/PPCISelLowering.cpp#L15570-L15591

This is interchanging and(any_ext(x),c) to any_ext(and(x,c)), which is obviously incorrect and should be zext(and(x,c)) instead.

@cuviper
Copy link
Member Author

cuviper commented Oct 10, 2023

That case is new in 17, llvm/llvm-project@b0e249d, so it would seem to be a true regression, not just something that we were lucky to miss before due to other perturbations.

@cuviper
Copy link
Member Author

cuviper commented Oct 10, 2023

This is interchanging and(any_ext(x),c) to any_ext(and(x,c)), which is obviously incorrect and should be zext(and(x,c)) instead.

Indeed, all datetime tests pass with:

diff --git a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
index 3ed0a261eb76..d4d2da55160e 100644
--- a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -15527,7 +15527,7 @@ SDValue PPCTargetLowering::PerformDAGCombine(SDNode *N,
       break;
     SDValue ConstOp = DAG.getConstant(Imm, dl, MVT::i32);
     SDValue NarrowAnd = DAG.getNode(ISD::AND, dl, MVT::i32, NarrowOp, ConstOp);
-    return DAG.getAnyExtOrTrunc(NarrowAnd, dl, N->getValueType(0));
+    return DAG.getZExtOrTrunc(NarrowAnd, dl, N->getValueType(0));
   }
   case ISD::SHL:
     return combineSHL(N, DCI);

This also passes all check-llvm-codegen-powerpc, including that original and-extend-combine.ll. I haven't tried to add or update a specific test for this problem yet.

@nikic
Copy link
Contributor

nikic commented Oct 11, 2023

Upstream issue: llvm/llvm-project#68783

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 11, 2023
@cuviper cuviper added the O-PowerPC Target: PowerPC processors label Oct 11, 2023
@bors bors closed this as completed in ca89f73 Oct 18, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-PowerPC Target: PowerPC processors P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants