LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 47567 - Incorrect transformation: (llvm.maximum undef, %x) -> undef
Summary: Incorrect transformation: (llvm.maximum undef, %x) -> undef
Status: RESOLVED FIXED
Alias: None
Product: libraries
Classification: Unclassified
Component: Scalar Optimizations (show other bugs)
Version: trunk
Hardware: PC All
: P enhancement
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks: 47948
  Show dependency tree
 
Reported: 2020-09-17 16:23 PDT by Zhengyang Liu
Modified: 2020-10-23 18:33 PDT (History)
10 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Zhengyang Liu 2020-09-17 16:23:04 PDT
Test case from Transforms/InstSimplify/floating-point-arithmetic.ll

define <2 x double> @maxnum_nan_op0_vec(<2 x double> %x) {
; CHECK-LABEL: @maxnum_nan_op0_vec(
; CHECK-NEXT:    ret <2 x double> [[X:%.*]]
;
  %r = call <2 x double> @llvm.maxnum.v2f64(<2 x double> <double 0x7ff8000000000000, double undef>, <2 x double> %x)
  ret <2 x double> %r
}

The second lane for this case performs the rewrite (llvm.maximum undef, %x) -> undef. The rewrite is illegal. The domain for (llvm.maximum undef, %x) depends on %x, and obviously the result of llvm.maximum does not fully cover the range of undef in most of cases.
Comment 1 Zhengyang Liu 2020-09-17 16:41:56 PDT
Transformation in alive2: https://alive2.llvm.org/ce/z/7ef9X7
Comment 2 Johannes Doerfert 2020-09-17 17:27:07 PDT
I guess we can make it `max(undef, x) => x` though.
Comment 3 Zhengyang Liu 2020-09-17 18:58:27 PDT
Sorry, I pasted the wrong code. The test case in the Description is correct.

Here's the buggy one (from Transforms/InstSimplify/floating-point-arithmetic.ll):

define <2 x double> @maximum_nan_op0_vec(<2 x double> %x) {
; CHECK-LABEL: @maximum_nan_op0_vec(
; CHECK-NEXT:    ret <2 x double> <double 0x7FF8000000000000, double undef>
;
  %r = call <2 x double> @llvm.maximum.v2f64(<2 x double> <double 0x7ff8000000000000, double undef>, <2 x double> %x)
  ret <2 x double> %r
}
Comment 4 Zhengyang Liu 2020-09-17 19:02:43 PDT
(In reply to Johannes Doerfert from comment #2)
> I guess we can make it `max(undef, x) => x` though.

Alive2 thinks so :), https://alive2.llvm.org/ce/z/Pzdatp
Comment 5 Sanjay Patel 2020-09-18 06:50:59 PDT
I'm not sure if it's better, but we are trying to return NAN (rather than the variable operand) in that test, and that should be correct:
https://alive2.llvm.org/ce/z/ysU7Ci

So we matched the partially-defined constant as a NAN, we just failed to propagate the fully-defined NAN from that.
Comment 6 Sanjay Patel 2020-09-18 08:05:15 PDT
https://github.com/llvm/llvm-project/commit/3f100e64b429b6468e9a2c5b3e7ef7757a06dc48

I also see that constant folding isn't fully implemented for any 2-operand FP intrinsics, so at minimum we have some inconsistent behavior (not sure if there are any miscompiles).
Comment 7 Johannes Doerfert 2020-09-18 08:32:48 PDT
(In reply to Sanjay Patel from comment #5)
> I'm not sure if it's better, but we are trying to return NAN (rather than
> the variable operand) in that test, and that should be correct:
> https://alive2.llvm.org/ce/z/ysU7Ci
> 
> So we matched the partially-defined constant as a NAN, we just failed to
> propagate the fully-defined NAN from that.

looks even better :)