[PATCH] D103169: [FPEnv][InstSimplify] Constrained FP support for NaN

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

kpn added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4900
+                 RoundingMode Rounding = RoundingMode::NearestTiesToEven) {
+  if (IsDefaultFPEnvironment(ExBehavior, Rounding))
+    if (Constant *C = foldOrCommuteConstant(Instruction::FSub, Op0, Op1, Q))
----------------
sepavloff wrote:
> Is default environment is necessary to make constant folding? "fpexcept.maytrap" also allows constant evaluation and any rounding except dynamic does not prevent the evaluation. Maybe check only for `ExBehavior != fp::ebStrict`?
Well, we'd also need to pass down the exception behavior because it'd be a shame to not fold 2.0+2.0, but we do want to avoid folding to (for example) infinity in the ebStrict case. So constant folding is something I'm hoping to put off for a later patch. This patch is intended to be restricted to NaN.

The codepath for the normal fsub and other normal FP instructions don't have any metadata to pass in. Thus they will pick up the default arguments in this function which is for the default FP environment. So the call to IsDefaultFPEnvironment() is a check for either the normal FP instruction or a constrained instruction that is supposed to behave the same as the normal FP instruction. Put another way, this check for the default FP environment preserves the existing behavior and avoids risking another source of bugs that could trigger a revert.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4951
+                              RoundingMode Rounding) {
+  if (Constant *C = simplifyFPOp({Op0, Op1}, FMF, Q, ExBehavior, Rounding))
     return C;
----------------
sepavloff wrote:
> In contrast to other `Simplify*` functions there is no call to `foldOrCommuteConstant`. May it be put here?
This function only gets called from SimplifyFMulInst(), and that function does have the call to foldOrCommuteConstant(). I'm guessing that's why this code doesn't already have the call in it. Since adding it wouldn't change anything I think we should leave it alone, at least in this patch.


================
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]]
----------------
spatel wrote:
> kpn wrote:
> > 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.
> Sorry for not seeing this and possibly other inline comments sooner.
> 
> The lack of poison propagation here is a TODO item (inconsistency bug), so let's try to get this out of the way before it gets more complicated. I'll post a patch for review shortly.
> 
> The history is that poison did not exist when I added the fold for undef. Undef behavior is more complicated than poison, (and that's why we're phasing it out).
> 
> We can't do "fadd x, undef --> undef" because if we deduce something about the variable input x, the result may not take on *any* possible FP value. This is similar to why "and x, undef --> 0" (not undef).
I'll change this patch when D104383 lands.


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

https://reviews.llvm.org/D103169