[PATCH] D103169: [FPEnv][InstSimplify] Constrained FP support for undef, poison, and NaN

[Kevin P. Neal via Phabricator via llvm-commits]

kpn added a comment.

In D103169#2782902 <https://reviews.llvm.org/D103169#2782902>, @spatel wrote:

> There's a lot going on here. I think we need to break this up to make sure we're testing the corner cases.

The meat of this is in InstructionSimplify.cpp's simplifyFPOp() and propagateNaN(). I don't see how we can split this up on the undef/poison/NaN axis without complicating those functions.

I could split this up on the fadd/fsub/fmul/etc axis and only handle constrained fadd, for example. Would that be helpful? Your current thoughts, @spatel?



================
Comment at: llvm/include/llvm/Analysis/InstructionSimplify.h:161
 Value *SimplifyFAddInst(Value *LHS, Value *RHS, FastMathFlags FMF,
-                        const SimplifyQuery &Q);
+                        const SimplifyQuery &Q, bool IsDefaultFPEnv = true,
+                        fp::ExceptionBehavior ExBehavior = fp::ebIgnore);
----------------
sepavloff wrote:
> Does it make sense to pass rounding mode instead of `IsDefaultFPEnv` and deduce if environment is default inside the function? Could it be useful for other foldings?
I believe you are correct. I'm passing the bool IsDefaultFPEnv here because the current easy way to check for the default environment is to ask the ConstrainedIntrinsic about the environment. One can just look at the rounding mode and exception handling oneself, but that check is a little too complicated for my taste to have littered all over the place.

How about a llvm::fp::IsDefaultFPEnv() function that takes the rounding mode and exception behavior and does the check itself? It's more readable to someone who hasn't been following along for the past three years. It's more concise. And it would let us replace this bool with an actual rounding mode argument.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4821
 
-    if (IsUndef || IsNan)
+    if ((IsUndef || IsNan) && ExBehavior != fp::ebStrict)
       return propagateNaN(cast<Constant>(V));
----------------
sepavloff wrote:
> Even if the exception behavior is `ebStrict` NaN propagation is not allowed only for signaling NaN, quiet NaN propagates quietly, without setting FP exceptions. Does it make sense to distinguish SNaNs and drop NaN propagation only for them?
For ebStrict we leave the instruction alone and let the run-time environment handle it.

A check of IEEE 754 seems to say that, when given an SNaN, an operation that produces a result will produce a QNaN. But I'm not a 754 expert. Anyone else? Please?

If you look above at the propagateNaN() function you'll see that someone thought of this and didn't have an answer. They left a comment asking the question. Was that @spatel?

I don't think a conversion to QNaN should be confined to the constrained intrinsics. It would be surprising if, for example, someone changed to a different rounding mode and suddenly NaN handling changed. So I think we should keep the codepath here the same for constrained and regular floating point. And changing propagateNaN() should be left for a later patch. Does that sound reasonable?


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4841-4842
 
+  if (!IsDefaultFPEnv)
+    return nullptr;
+
----------------
sepavloff wrote:
> With the exception of `fadd X, -0`, which may produce different results for different rounding modes, other simplifications seem to be useful in non-default environment. Which of the cases cannot be made in such environment?
I don't know. I didn't want to put too much into one patch so I just took the conservative approach and bailed out here. If I made an error with these optimizations then a straight revert of the patch would make a mess. 

I figure we can revisit later?


================
Comment at: llvm/test/Transforms/InstSimplify/fp-undef-poison-strictfp.ll:21
+; CHECK-LABEL: @fadd_undef_op0_maytrap(
+; CHECK-NEXT:    ret float 0x7FF8000000000000
+;
----------------
sepavloff wrote:
> Should this call produce undef value instead?
This comes from propagateNaN(), which spins up a NaN if handed a value  that is not a NaN. That's where the undef is changed into a NaN.

Since an undef is the set of all possible bit patterns then that set includes NaN. If I understand undef correctly we're allowed to pick a value from that set and use it. So the conversion of undef into NaN is allowed.

Plus, the current code does this already. So I'm inclined to leave it alone and have this conversion produce NaN. 


================
Comment at: llvm/test/Transforms/InstSimplify/fp-undef-poison-strictfp.ll:45
+; CHECK-LABEL: @fadd_poison_op0_strict(
+; CHECK-NEXT:    [[R:%.*]] = call float @llvm.experimental.constrained.fadd.f32(float poison, float [[X:%.*]], metadata !"round.dynamic", metadata !"fpexcept.strict") #[[ATTR0]]
+; CHECK-NEXT:    ret float [[R]]
----------------
sepavloff wrote:
> According to documentation (https://llvm.org/docs/LangRef.html#poison-values) "An instruction that depends on a poison value, produces a poison value itself". So making call here is incorrect, it would result in lost of poison.
Yes, but the very next sentence is "A poison value may be relaxed into an undef value, ...".

Using the regular instructions we have this difference:
add x, poison -> poison
fadd x, poison -> NaN

I don't know exactly why, but that's the existing practice. The constrained intrinsics should be the same as the regular FP instructions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103169/new/

https://reviews.llvm.org/D103169