[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());
+
----------------
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


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

https://reviews.llvm.org/D149587