[PATCH] D149587: InstSimplify: Simplifications for ldexp

[Joshua Cranmer via Phabricator via llvm-commits]

jcranmer-intel added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:6112
+  // TODO: Could fold this if we know the exception mode isn't strict, we know
+  // the rounding mode, denormal mode and other target modes.
+  if (match(Op1, PatternMatch::m_ZeroInt()))
----------------
Rounding mode doesn't come into play, since ldexp is always an exact operation.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:6084
+    if (Q.isUndefValue(Op0))
+      return ConstantFP::getNaN(Op0->getType());
+
----------------
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.


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

https://reviews.llvm.org/D149587