[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:
> 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?
> How about adding a helper function to deal with ConstrainedFP and calling it from the very first clause in this function?

The shared code is moved to `getEvaluationRoundingMode`.

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

Constrained intrinsics should not differ from their counterparts in handling undef/poison. @kpn is working on D103169, which eventually should implement the relevant folding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102673