[PATCH] D102673: [ConstantFolding] Fold constrained arithmetic intrinsics

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:2408
   if (auto *Op1 = dyn_cast<ConstantFP>(Operands[0])) {
-    if (!Ty->isHalfTy() && !Ty->isFloatTy() && !Ty->isDoubleTy())
+    if (!Ty->isFloatingPointTy())
       return nullptr;
----------------
sepavloff wrote:
> spatel wrote:
> > From the tests, I'm assuming that this diff is here to support bfloat, but this change affects more than constrained intrinsics and more than only bfloat. 
> > 
> > Either we need to limit this enhancement, or we need to split this into its own patch and add more tests to make sure it works as expected. I put in some basic coverage for copysign here:
> > 8a4d05ddb3ff
> > 
> > I think things work as expected for simple cases like that one, but I don't know what happens if we are using the host mathlib to evaluate more complex functions (for example "pow" is in the switch under here).
> I put checks
> ```
> if (!Ty->isHalfTy() && !Ty->isFloatTy() && !Ty->isDoubleTy())
>         return nullptr;
> ```
> below. Constrained intrinsics have tests for bfloat type in this patch. As for other functions, test for them will be added later, and these checks will be removed.
Thanks. I'm worried that someone could come back after this patch and add yet another `if` or `switch` and miss the differences in type handling. 

How about adding a helper function to deal with ConstrainedFP and calling it from the very first clause in this function? 

That also raises a question: what is the behavior of these intrinsics with undef and/or poison operands?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102673