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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff 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;
----------------
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.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:2449
+          // something definite.
+          if (*ORM == RoundingMode::Dynamic)
+            ORM = RoundingMode::NearestTiesToEven;
----------------
foad wrote:
> You don't need the "if", just do it unconditionally?
Yes, you are right. Updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102673