Page MenuHomePhabricator

[ConstantFold] fold fsub -0.0, undef to undef rather than NaN
ClosedPublic

Authored by spatel on Feb 17 2020, 6:50 AM.

Details

Summary

A question about this behavior came up on llvm-dev:
http://lists.llvm.org/pipermail/llvm-dev/2020-February/139003.html
...and as part of backend improvements in D73978, but let's see if we can reach agreement on the transforms in IR first because we already have fairly thorough tests in place here.

If one constant operand to {fadd/fsub/fmul/fdiv/frem} is not {NaN/Inf/0.0/-0.0} and the other constant operand is undef, then we can choose 'undef' as some other value that changes all of the fields (sign, exponent, fraction) in the result FP value. Is that enough to say the result is undef?

Diff Detail

Event Timeline

spatel created this revision.Feb 17 2020, 6:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2020, 6:50 AM

Is that enough to say the result is undef?

Theoretically, no. To take a simple integer example, suppose you have "max(undef, 1)". You can toggle any of the bits separately, but you can't produce 0, so converting it to "undef" is wrong. Alive2 does these sorts of proofs if you want mechanical verification (although I don't think it handles fp).

Practically, I doubt anyone would notice if "1e100+undef" produced an impossible result.

lebedev.ri requested changes to this revision.Feb 17 2020, 11:59 AM
lebedev.ri added inline comments.
llvm/test/Analysis/ConstantFolding/vector-undef-elts.ll
46–52
----------------------------------------
define <3 x float> @fadd() {
%0:
  %c = fadd <3 x float> { undef, 42.000000, undef }, undef
  ret <3 x float> %c
}
=>
define <3 x float> @fadd() {
%0:
  ret <3 x float> undef
}
Transformation doesn't verify!
ERROR: Value mismatch

Example:

Source:
<3 x float> %c = < undef, #x42280000 (42)       [based on undef value], NaN     [based on undef value] >

Target:
Source value: < NaN     [based on undef value], #x42280000 (42), NaN >
Target value: < #x00000000 (+0.0), #x00000020 (0.000000000000?), NaN >

Summary:
  0 correct transformations
  1 incorrect transformations
  0 errors
This revision now requires changes to proceed.Feb 17 2020, 11:59 AM
lebedev.ri added inline comments.Feb 17 2020, 12:03 PM
llvm/test/Transforms/InstSimplify/fp-undef.ll
211–212

This is correct

----------------------------------------
define float @fadd_undef_op0_nnan_constant(float %x) {
%0:
  %r = fadd nnan float undef, 1.000000
  ret float %r
}
=>
define float @fadd_undef_op0_nnan_constant(float %x) {
%0:
  %r = fadd nnan float undef, 1.000000
  ret float undef
}
Transformation seems to be correct!

Summary:
  1 correct transformations
  0 incorrect transformations
  0 errors
218–224

This is not correct

----------------------------------------
define float @fadd_undef_op1_constant(float %x) {
%0:
  %r = fadd float 2.000000, undef
  ret float %r
}
=>
define float @fadd_undef_op1_constant(float %x) {
%0:
  %r = fadd float 2.000000, undef
  ret float undef
}
Transformation doesn't verify!
ERROR: Value mismatch

Example:
float %x = poison

Source:
float %r = NaN  [based on undef value]

Target:
float %r = NaN  [based on undef value]
Source value: NaN
Target value: #x84000400 (-0.000000000000?)

Summary:
  0 correct transformations
  1 incorrect transformations
  0 errors
234–240

Again, no

----------------------------------------
define float @fsub_undef_op1_constant(float %x) {
%0:
  %r = fsub float 4.000000, undef
  ret float %r
}
=>
define float @fsub_undef_op1_constant(float %x) {
%0:
  %r = fsub float 4.000000, undef
  ret float undef
}
Transformation doesn't verify!
ERROR: Value mismatch

Example:
float %x = poison

Source:
float %r = NaN  [based on undef value]

Target:
float %r = NaN  [based on undef value]
Source value: NaN
Target value: #x84000400 (-0.000000000000?)

Summary:
  0 correct transformations
  1 incorrect transformations
  0 errors
242–248

This is correct

250–256

No

----------------------------------------
define float @fmul_undef_op1_constant(float %x) {
%0:
  %r = fmul float 6.000000, undef
  ret float %r
}
=>
define float @fmul_undef_op1_constant(float %x) {
%0:
  %r = fmul float 6.000000, undef
  ret float undef
}
Transformation doesn't verify!
ERROR: Value mismatch

Example:
float %x = poison

Source:
float %r = NaN  [based on undef value]

Target:
float %r = NaN  [based on undef value]
Source value: NaN
Target value: #x00001000 (0.000000000000?)

Summary:
  0 correct transformations
  1 incorrect transformations
  0 errors
nikic added a subscriber: nikic.Feb 17 2020, 12:32 PM
spatel marked an inline comment as done.Feb 17 2020, 1:02 PM
spatel added inline comments.
llvm/test/Transforms/InstSimplify/fp-undef.ll
218–224

Apologies for not having recent Alive installed anywhere at the moment to test this myself, but how should we interpret this output?

Is it saying that there is no value that we can assign to the undef operand that will produce the exact 0x84000400 output value?

nlopes added inline comments.Feb 17 2020, 2:30 PM
llvm/test/Transforms/InstSimplify/fp-undef.ll
218–224

Exactly!
"2.0 + x" can never be -1.5048164E-36 (0x84000400) for any x.

Is that enough to say the result is undef?

Theoretically, no. To take a simple integer example, suppose you have "max(undef, 1)". You can toggle any of the bits separately, but you can't produce 0, so converting it to "undef" is wrong. Alive2 does these sorts of proofs if you want mechanical verification (although I don't think it handles fp).

Practically, I doubt anyone would notice if "1e100+undef" produced an impossible result.

That's what I was expecting: each FP opcode has exact bit patterns that can never be created given one known constant operand. However, we do not have the FP analysis to determine those patterns in LLVM currently, and even if we did, there's no practical way to use that knowledge for optimization.

Having these fold to undef would make things like D73978 and vector demanded elements easier, but if the consensus is to keep things theoretically pure, we'll have to work around the NaN folding.

spatel updated this revision to Diff 245734.Feb 20 2020, 1:55 PM
spatel retitled this revision from [ConstantFold] fold most FP ops with undef operand to undef rather than NaN to [ConstantFold] fold fsub -0.0, undef to undef rather than NaN.

Patch updated:
Only handle the fneg-like form of fsub.

lebedev.ri accepted this revision.Feb 20 2020, 2:40 PM

This is fine in general for half/float/double for fsub nnan float undef, %x/fsub nnan float %x, undef.

This revision is now accepted and ready to land.Feb 20 2020, 2:40 PM
This revision was automatically updated to reflect the committed changes.