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

Incorrect transformation of minnum with nnan flag #44823

Closed
zhengyang92 opened this issue Apr 8, 2020 · 11 comments
Closed

Incorrect transformation of minnum with nnan flag #44823

zhengyang92 opened this issue Apr 8, 2020 · 11 comments
Assignees
Labels
bugzilla Issues migrated from bugzilla

Comments

@zhengyang92
Copy link
Contributor

Bugzilla Link 45478
Resolution FIXED
Resolved on Apr 24, 2020 09:46
Version trunk
OS All
CC @efriedma-quic,@ecnelises,@aqjune,@LebedevRI,@zhengyang92,@nunoplopes,@regehr,@rotateright,@yuanfang-chen
Fixed by commit(s) e4175ff

Extended Description

As stated in LangRef, if either operand for llvm.minnum is a NaN, returns the other non-NaN operand. If llvm.minnum is attached with nnan flag, when an argument is a NaN, it produces a poison value.

See below the incorrect transformation. When input %x is NaN, the source program returns 0.0, while the target program returns poison. Target is more poisonous than source.

llvm/test/Transforms/InstCombine/minnum.ll

define float @​minnum_f32_1_minnum_p0_val_nnan_ninf(float %x) {
; CHECK-LABEL: @​minnum_f32_1_minnum_p0_val_nnan_ninf(
; CHECK-NEXT: [[TMP1:%.]] = call nnan ninf float @​llvm.minnum.f32(float [[X:%.]], float 0.000000e+00)
; CHECK-NEXT: ret float [[TMP1]]
;
%y = call float @​llvm.minnum.f32(float 0.0, float %x)
%z = call nnan ninf float @​llvm.minnum.f32(float %y, float 1.0)
ret float %z
}

@zhengyang92
Copy link
Contributor Author

assigned to @rotateright

@rotateright
Copy link
Contributor

This is a reassociation transform in InstCombiner::visitCallInst():
// m(m(X, C2), C1) -> m(X, C)

It's propagating flags from the 2nd (outer) min/max, but needs to intersect them instead?

Online version of Alive2 doesn't seem to know about minnum intrinsic:
http://volta.cs.utah.edu:8080/z/BvUNKD

@regehr
Copy link
Contributor

regehr commented Apr 8, 2020

sorry, yes, this patch hasn't landed and I haven't setup our compiler explorer to use branches -- will update as soon as it lands

@zhengyang92
Copy link
Contributor Author

This is a reassociation transform in InstCombiner::visitCallInst():
// m(m(X, C2), C1) -> m(X, C)

It's propagating flags from the 2nd (outer) min/max, but needs to intersect
them instead?

Online version of Alive2 doesn't seem to know about minnum intrinsic:
http://volta.cs.utah.edu:8080/z/BvUNKD

Posting alive2 output if you're interested.

define float @​minnum_f32_1_minnum_p0_val_nnan_ninf(float %x) {
%0:
%y = fmin float 0.000000, %x
%z = fmin nnan ninf float %y, 1.000000
ret float %z
}
=>
define float @​minnum_f32_1_minnum_p0_val_nnan_ninf(float %x) {
%0:
%1 = fmin nnan ninf float %x, 0.000000
ret float %1
}
Transformation doesn't verify!
ERROR: Target is more poisonous than source

Example:
float %x = NaN

Source:
float %y = #x00000000 (+0.0)
float %z = #x00000000 (+0.0)

Target:
float %1 = poison
Source value: #x00000000 (+0.0)
Target value: poison

@regehr
Copy link
Contributor

regehr commented Apr 9, 2020

our compiler explorer supports this test case now:

http://volta.cs.utah.edu:8080/z/BAXhiL

@ecnelises
Copy link
Member

This also applies to maxnum.

Anyone has been working on it? If not, I'm glad to take this

@rotateright
Copy link
Contributor

This also applies to maxnum.

Anyone has been working on it? If not, I'm glad to take this

Sorry, I lost track of this bug. I have a draft somewhere that would intersect the flags. Let me clean that up and post for review.

@rotateright
Copy link
Contributor

Alive2 (online version at least) doesn't recognize the related "minimum" and "maximum" intrinsics?
http://volta.cs.utah.edu:8080/z/QoZRRC

declare float @​llvm.maximum.f32(float, float)

define float @​src(float %x) {
%y = call float @​llvm.maximum.f32(float 0.0, float %x)
%z = call float @​llvm.maximum.f32(float %y, float 1.0)
ret float %z
}

define float @​tgt(float %x) {
%z = call float @​llvm.maximum.f32(float %x, float 1.0)
ret float %z
}

@rotateright
Copy link
Contributor

@zhengyang92
Copy link
Contributor Author

Hi Sanjay,

These two are WIP and will be supported soon.

Alive2 (online version at least) doesn't recognize the related "minimum" and
"maximum" intrinsics?

@rotateright
Copy link
Contributor

Should be fixed with:
https://reviews.llvm.org/rGe4175ff52561

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

4 participants