[PATCH] D112256: [FPEnv][EarlyCSE] Add support for CSE of constrained FP intrinsics, take 2

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I could use a high-level refresher (more info in the patch description) - why do we need to defer replacement on constrained intrinsics? The answer might be visible in that last set of tests, but I am not sure.



================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:1388
+      if (auto *CI = dyn_cast<ConstrainedFPIntrinsic>(&Inst)) {
+        assert(CI->getExceptionBehavior().getValue() != fp::ebStrict &&
+               "Unexpected ebStrict from SimpleValue::canHandle()");
----------------
spatel wrote:
> Add another assert that RoundingMode (if present) is not Dynamic?
Is it possible that the optional ExceptionBehavior was not present?


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:1388-1389
+      if (auto *CI = dyn_cast<ConstrainedFPIntrinsic>(&Inst)) {
+        assert(CI->getExceptionBehavior().getValue() != fp::ebStrict &&
+               "Unexpected ebStrict from SimpleValue::canHandle()");
+        if (CI->getExceptionBehavior().getValue() == fp::ebMayTrap) {
----------------
Add another assert that RoundingMode (if present) is not Dynamic?


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:1430-1431
+      if (!DeferredElideWith) {
+        AvailableValues.insert(SimpleValue(&Inst),
+                               &Inst);
+        continue;
----------------
formatting


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:1638
+        // instructions.
+        if (isa<FPMathOperator>(I) || (I->hasPoisonGeneratingFlags() && !programUndefinedIfPoison(I)))
+          I->andIRFlags(&Inst);
----------------
formatting


================
Comment at: llvm/test/Transforms/EarlyCSE/tfpropagation.ll:5
+
+define i64 @branching_int(i32 %a) {
+; CHECK-LABEL: @branching_int(
----------------
I'm not sure what changes we want to see in this set of tests - please add a comment like the other files have.


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

https://reviews.llvm.org/D112256