[PATCH] D72930: [FEnv] Constfold some unary constrained operations

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1407
   switch (F->getIntrinsicID()) {
   case Intrinsic::fabs:
   case Intrinsic::minnum:
----------------
sepavloff wrote:
> evandro wrote:
> > Should these cases be true even when `isStrictFP()` is true?
> `fabs` is allowed irrespective of `isStrictFP()` , so it should be processed here. As for functions like `ceil`, they cannot be found in strictfp function, corresponding operations are represented by constrained intrinsics.
I originally added the Call->isStrictFP() check here before we had constrained versions of the primary FP intrinsics. Now that we have them it should be OK to let those pass. However, we aren't yet enforcing the rule that all calls within a strictfp function must be the constrained versions, so it might be too soon to make that assumption here.

Also, I don't think we're planning to have constrained versions of the target specific intrinsics like Intrinsic::x86_avx512_vcvtss2usi32. Those will probably only have the strictfp attribute on the callsite and possibly an operand bundle to specify the specific details. The other intrinsics might revert to behavior like that too some day.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1479
   case Intrinsic::x86_avx512_cvttsd2usi64:
   case Intrinsic::is_constant:
+  case Intrinsic::experimental_constrained_ceil:
----------------
evandro wrote:
> Or should...
> 
> 
> ```
> return !Call->isStrictFP();
> ```
> 
> be inserted here?
This is a reasonable suggestion.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1822
+      if (U.isFinite()) {
+        U.roundToIntegral(*RM);
+      } else if (U.isSignaling()) {
----------------
For all of these except nearbyint, you need to check the return code and only remove the call if no flags are raised or exceptionBehavior is ignore. The rest of the functions raise the inexact flag if the value is not already an integer.

We could both keep the call and RAUW the return value. We need the exception to be raised, but the constant folding could unlock additional optimizations. It might be useful to introduce new intrinsics like llvm.raiseinexact() so that we could later combine multiple calls raising the same flag.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:2526
                                  const TargetLibraryInfo *TLI) {
-  if (Call->isNoBuiltin() || Call->isStrictFP())
+  if (Call->isNoBuiltin())
     return nullptr;
----------------
sepavloff wrote:
> evandro wrote:
> > Again, not sure about the impact of doing this...
> The same thing. If an operation depends on or changes current floating point environment, it is represented by corresponding constrained intrinsics in strictfp function. 
This is probably OK. I think these changes will always end up going through the other function for FP calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72930