Incorrect transformation: (llvm.maximum undef, %x) -> undef

[zhengyang92]

Bugzilla Link	47567
Resolution	FIXED
Resolved on	Oct 23, 2020 18:33
Version	trunk
OS	All
Blocks	#47292
CC	@efriedma-quic,@jdoerfert,@aqjune,@LebedevRI,@zhengyang92,@RKSimon,@nunoplopes,@regehr,@rotateright

Extended Description

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.

[zhengyang92]

Transformation in alive2: https://alive2.llvm.org/ce/z/7ef9X7

[jdoerfert]

I guess we can make it max(undef, x) => x though.

[zhengyang92]

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
}

[zhengyang92]

I guess we can make it max(undef, x) => x though.

Alive2 thinks so :), https://alive2.llvm.org/ce/z/Pzdatp

[rotateright]

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.

[rotateright]

3f100e6

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).

[jdoerfert]

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 :)

[aqjune]

mentioned in issue #47292