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

Wed Jun 16 08:37:37 PDT 2021

spatel added inline comments.

================
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]]
----------------
kpn wrote:
> 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.
Thanks - ce95200b7942
Can you also pre-commit the tests with the baseline CHECK lines? That would make it easier to see what is (and what is not) changing with this patch.

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

https://reviews.llvm.org/D103169