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

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

kpn added a comment.

I'll update the patch later today with that missing assertion.



================
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:
> spatel wrote:
> > Add another assert that RoundingMode (if present) is not Dynamic?
> Is it possible that the optional ExceptionBehavior was not present?
No. I believe all constrained intrinsics specify an exception behavior.


================
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) {
----------------
kpn wrote:
> spatel wrote:
> > spatel wrote:
> > > Add another assert that RoundingMode (if present) is not Dynamic?
> > Is it possible that the optional ExceptionBehavior was not present?
> No. I believe all constrained intrinsics specify an exception behavior.
Will do.


================
Comment at: llvm/test/Transforms/EarlyCSE/tfpropagation.ll:125
 
 define double @branching_maytrap(i64 %a) #0 {
 ; CHECK-LABEL: @branching_maytrap(
----------------
spatel wrote:
> Am I seeing this correctly - all tests now transform?
> Should there be tests where one or both of the constrained ops is "fpexcept.strict"?
> Please add some explanatory comments for these tests.
I precommitted the tests. The tests that aren't showing up in the diff now were the negative tests that included "fpexcept.strict" and the dynamic rounding tests. Since those test for changes that must not happen, this patch here shows no changes to those tests.

What sort of explanatory comments do you have in mind?


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

https://reviews.llvm.org/D112256