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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff 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;
----------------
nikic wrote:
> kpn wrote:
> > 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.
> Analysis passes are indeed not allowed to change the IR. And this isn't a harmless change either if it gets left behind -- e.g. it invalidates MemorySSA.
> Analysis passes are indeed not allowed to change the IR.

Constant folding is not an analysis, it changes IR. I don't know why this file is in Analysis directory.

>What if the caller decides to not do a fold after calling this function to see if a fold is possible?

Nothing bad would happen. Only side effect would be removed, but we know for sure that it is absent, we just evaluated the operation. The function call would not be removed if its result is used.

> And why is the ebIgnore case different from the other two?

If exception behavior is `ebIgnore`, such calls will get attribute `SDNodeFlags::NoFPExcept` in DAG and such instructions do not have side effects. But setting `ReadNone` for instructions with `ebIgnore` allows removal of these instructions earlier, at IR level, which could have positive effect.

> And this isn't a harmless change either if it gets left behind -- e.g. it invalidates MemorySSA.

Comparison of two floating point numbers do not use memory access. But it can change bits in the floating point state register (only Invalid bit can be set). This change is emulated as memory access so that instruction be ordered correctly. This is why constrained intrinsics declared with attribute `IntrInaccessibleMemOnly`. As no actual memory access occurs, it is harmless to set `ReadNone` in this case.



================
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);
----------------
kpn wrote:
> Besides moving this code to a new function, are there any changes to it? It's hard to tell.
No, this is only a code moving.


================
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 {
----------------
kpn wrote:
> 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.
Added tests with "maytrap".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110322