[PATCH] D103169: [FPEnv][InstSimplify] Constrained FP support for undef, poison, and NaN

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added a comment.

Could you please split this patch so that folding Nans would be separated from treatment of poison and undefs?

In contrast to poison and undef, Nan is a real value, which has definite representation in runtime, it is clear how to process it. Poison value is a way to propagate errors such as undefined behavior. It exists only in compile time. Of course, lowering it to NaN in instruction selector is natural, but conversion of poison into NaN in IR transformation looks like incorrect decision except some cases (like NaN*poison -> NaN).  Yes, the documentation mentions that "A poison value may be relaxed into an undef value", but it is unclear when it is allowed. Undefs are even more obscure. The use case of input data, which should not influence result of an operation is clear but undefs are used in much more cases. I feel review of such transformation could be more complex.

As for Nans, such transformation is useful and anticipated.



================
Comment at: llvm/include/llvm/Analysis/InstructionSimplify.h:161
 Value *SimplifyFAddInst(Value *LHS, Value *RHS, FastMathFlags FMF,
-                        const SimplifyQuery &Q);
+                        const SimplifyQuery &Q, bool IsDefaultFPEnv = true,
+                        fp::ExceptionBehavior ExBehavior = fp::ebIgnore);
----------------
kpn wrote:
> sepavloff wrote:
> > Does it make sense to pass rounding mode instead of `IsDefaultFPEnv` and deduce if environment is default inside the function? Could it be useful for other foldings?
> I believe you are correct. I'm passing the bool IsDefaultFPEnv here because the current easy way to check for the default environment is to ask the ConstrainedIntrinsic about the environment. One can just look at the rounding mode and exception handling oneself, but that check is a little too complicated for my taste to have littered all over the place.
> 
> How about a llvm::fp::IsDefaultFPEnv() function that takes the rounding mode and exception behavior and does the check itself? It's more readable to someone who hasn't been following along for the past three years. It's more concise. And it would let us replace this bool with an actual rounding mode argument.
> How about a llvm::fp::IsDefaultFPEnv() function that takes the rounding mode and exception behavior and does the check itself?

I like this idea. In general it is profitable to have interface that can be extended.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4841-4842
 
+  if (!IsDefaultFPEnv)
+    return nullptr;
+
----------------
kpn wrote:
> sepavloff wrote:
> > With the exception of `fadd X, -0`, which may produce different results for different rounding modes, other simplifications seem to be useful in non-default environment. Which of the cases cannot be made in such environment?
> I don't know. I didn't want to put too much into one patch so I just took the conservative approach and bailed out here. If I made an error with these optimizations then a straight revert of the patch would make a mess. 
> 
> I figure we can revisit later?
It is Ok to postpone this issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103169