[PATCH] D110322: [ConstantFolding] Fold constrained compare intrinsics

[Kevin P. Neal via Phabricator via llvm-commits]

kpn added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1854
     if (EB && *EB != fp::ExceptionBehavior::ebIgnore)
-      CI->addFnAttr(Attribute::ReadNone);
+      const_cast<ConstrainedFPIntrinsic *>(CI)->addFnAttr(Attribute::ReadNone);
     return true;
----------------
This still worries me. Are _analysis_ passes allowed to change the input IR? What if the caller decides to not do a fold after calling this function to see if a fold is possible? And why is the ebIgnore case different from the other two?

Constant folding won't add this ReadNone attribute when the constant folding code doesn't have an Instruction to alter. I don't know what to do about that.


================
Comment at: llvm/lib/IR/Instructions.cpp:4134
+bool FCmpInst::evaluate(Predicate Pred, const APFloat &Op1,
+                        const APFloat &Op2) {
+  APFloat::cmpResult R = Op1.compare(Op2);
----------------
Besides moving this code to a new function, are there any changes to it? It's hard to tell.


================
Comment at: llvm/test/Transforms/InstSimplify/constfold-constrained.ll:418
 
+; When exceptions are ignored, comparison of constants can be folded, even for (signaling) NaNs.
+define i1 @cmp_eq_01() #0 {
----------------
There are no "maytrap" tests here. For the sake of future readers it would probably be useful to show that "maytrap" was taken into account and is working correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110322