llvm / llvm/llvm-project

LLVM broke frontends relying on documented NaN behavior for minnum/maxnum

Open
#170,082 19 comments 0 reactions 0 assignees View on GitHub
floating-point llvm:instcombine llvm:transforms
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

https://github.com/llvm/llvm-project/pull/139581 (specifically, 17efa572c3827fe12e37d0f1f28d20a8e44890c7) introduced a breaking change that affects Rust and possibly other frontends: we were relying on the documented, stable, established property that for `minnum`/`maxnum`,

> If either operand is a NaN, returns the other non-NaN operand.

Unfortunately, that documentation got changed in https://github.com/llvm/llvm-project/pull/112852, and then in 17efa572c3827fe12e37d0f1f28d20a8e44890c7 the implementation also got changed so now `maxnum(sNaN, x)` is simplified to `qNaN`. As a result of that, the code rustc+LLVM generate now no longer matches what we document in the Rust standard library. (This does not completely fix the implementation though. LLVM is currently implementing a messy mix of old and new semantics, which is being discussed in https://github.com/llvm/llvm-project/pull/138451, [Discourse](https://discourse.llvm.org/t/rfc-a-consistent-set-of-semantics-for-the-floating-point-minimum-and-maximum-operations/89006), and some other places. That's not what this issue is about; this issue is about the tangible effects on frontends.)

I consider this a regression introduced by #139581; IMO the problematic commit in that PR should be reverted. Changing the behavior of existing intrinsics used by frontends requires more care. Also, https://llvm.org/docs/LangRef.html#llvm-minimumnum-intrinsic exists for those that want the IEEE754-2019 semantics, so it's not clear to me why the old `minnum` intrinsic has to change. IIUC, https://github.com/llvm/llvm-project/commit/17efa572c3827fe12e37d0f1f28d20a8e44890c7 has not made it into any release yet, so there's still a chance of fixing this before it affects Rust users.

Cc @LewisCrawford @nikic @arsenm

Contributor guide

Open the contributing guide

Research direction

Start by reading PR 139581 and commit 17efa572c3827fe12e37d0f1f28d20a8e44890c7, then compare the documented minnum/maxnum behavior with the implementation and the linked LangRef intrinsic. Review the related PRs and Discourse discussion before deciding whether the frontend-facing regression can be resolved without changing established semantics; done means the documented behavior and generated frontend code agree before release.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.