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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
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);
----------------
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?


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4821
 
-    if (IsUndef || IsNan)
+    if ((IsUndef || IsNan) && ExBehavior != fp::ebStrict)
       return propagateNaN(cast<Constant>(V));
----------------
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?


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4841-4842
 
+  if (!IsDefaultFPEnv)
+    return nullptr;
+
----------------
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?


================
Comment at: llvm/test/Transforms/InstSimplify/fp-undef-poison-strictfp.ll:21
+; CHECK-LABEL: @fadd_undef_op0_maytrap(
+; CHECK-NEXT:    ret float 0x7FF8000000000000
+;
----------------
Should this call produce undef value instead?


================
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]]
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103169