[PATCH] D99962: [FPEnv] EarlyCSE support for constrained intrinsics, default FP environment edition

[Joseph Tremoulet via Phabricator via llvm-commits]

JosephTremoulet added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:120
+        switch((Intrinsic::ID)F->getIntrinsicID()) {
+          case Intrinsic::experimental_constrained_fadd:
+          case Intrinsic::experimental_constrained_fsub:
----------------
Should `doesNotAccessMemory` be returning true for these?  Then the existing code would pick them up.


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:1072
+  // using the default environment!
+  if (!SimpleValue::canHandle(CondInst))
+    return false;
----------------
I'm confused why you need this, I think the caller is already checking it (see comment at callsite).


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:1217
       if (CondInst && SimpleValue::canHandle(CondInst))
         Changed |= handleBranchCondition(CondInst, BI, BB, Pred);
     }
----------------
Isn't this the only call to `handleBranchCondition`?  It's already guarded by `canHandle(CondInst)`.


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

https://reviews.llvm.org/D99962