[PATCH] D149587: InstSimplify: Simplifications for ldexp

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:6084
+    if (Q.isUndefValue(Op0))
+      return ConstantFP::getNaN(Op0->getType());
+
----------------
foad wrote:
> arsenm wrote:
> > foad wrote:
> > > arsenm wrote:
> > > > arsenm wrote:
> > > > > jcranmer-intel wrote:
> > > > > > arsenm wrote:
> > > > > > > arsenm wrote:
> > > > > > > > foad wrote:
> > > > > > > > > arsenm wrote:
> > > > > > > > > > arsenm wrote:
> > > > > > > > > > > foad wrote:
> > > > > > > > > > > > arsenm wrote:
> > > > > > > > > > > > > foad wrote:
> > > > > > > > > > > > > > Why is this not strictfp-safe?
> > > > > > > > > > > > > If undef resolved to a signaling nan it wouldn't raise an exception
> > > > > > > > > > > > But here you are //choosing// what you want the undef value to be, so choose a quiet NaN.
> > > > > > > > > > > https://github.com/llvm/llvm-project/blob/67a212af4c24426de6e436e9b82590d41faa665c/llvm/lib/Analysis/InstructionSimplify.cpp#L5513
> > > > > > > > > > > 
> > > > > > > > > > > This is checking this which is a more refined check of strictfp
> > > > > > > > > > although really I should probably just call simplifyFPOp in the first place
> > > > > > > > > > https://github.com/llvm/llvm-project/blob/67a212af4c24426de6e436e9b82590d41faa665c/llvm/lib/Analysis/InstructionSimplify.cpp#L5513
> > > > > > > > > Well I don't understand why that code doesn't propagate quiet NaNs unconditionally. I agree with @sepavloff's comment: https://reviews.llvm.org/D103169#inline-979968
> > > > > > > > > 
> > > > > > > > > Also, that code handles all fp ops but here we only care specifically about ldexp. Where is the spec for what fp exceptions ldexp can raise? `man ldexp` mentions exceptions on overflow and underflow, but does not mention raising invalid operation even on a signalling NaN input.
> > > > > > > > > 
> > > > > > > > > In any case a comment explaining why each case is supposedly not fpstrict-safe would really help, since this stuff is massively non-obvious.
> > > > > > > > man ldexp says:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > >        Range error, overflow
> > > > > > > > >               errno is set to ERANGE.  An overflow floating-point exception (FE_OVERFLOW) is raised.
> > > > > > > > > 
> > > > > > > > >        Range error, underflow
> > > > > > > > >               errno is set to ERANGE.  An underflow floating-point exception (FE_UNDERFLOW) is raised.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > but does not mention raising invalid operation even on a signalling NaN input.
> > > > > > > 
> > > > > > > This is implied for every FP operation. There are just the exceptions for fabs/fneg/copysign/is.fpclass
> > > > > > I think for this block of code, just deferring to simplifyFPOp is better; the only thing that differs here is that ldexp(x, undef) needs to fold to x.
> > > > > simplifyFPOp complicates things because that's expecting only float inputs and here there's an integer op
> > > > I keep trying to make it use simplifyFPOp and I'm unhappy with it. It's ignoring the denorm flushing potential, uses FastMathFlags and i'm about 80% sure the precedent here for getting to the FMF is broken. I see various places using the CxtI in the SimplifyQuery, which is likely not the instruction the flags are actually attached to. It's not really less code to just handle the nan case directly while I have the APFloat
> > > I've read all the comments here and I still don't understand why this case is not strictfp-safe.
> > Folding to the input operand drops a canonicalize. It would be more correct to introduce llvm.experimental.constrained.canonicalize which does not exist
> You have a free choice of what input operand value to assume, so choose one for which llvm.experimental.constrained.canonicalize would be a no-op? Isn't that already true for the value returned by getNaN? (Or is this some MIPS weirdness again where we don't know whether that NaN is quiet or not?)
Maybe for the ldexp(undef, x) -> nan case (where we evidently don't have concrete rules for payload bits)

I'm talking about the ldexp(x, undef) -> x case



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

https://reviews.llvm.org/D149587