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

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

kpn 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:
----------------
JosephTremoulet wrote:
> Should `doesNotAccessMemory` be returning true for these?  Then the existing code would pick them up.
There are a lot of constrained intrinsics, but I wanted to be certain that I'm restricting the handling here to just these. I worry that if 'readonly' is set somewhere else then it could get changed in the future without anyone realizing that the behavior here would change. Thus the explicit list here.

Also, depending on the exception handling metadata argument, these constrained intrinsics may cause traps and they may change the FP status bits/register. In the default environment we assume no traps, and we assume nobody is looking at the FP status bits. But the default environment is communicated via the metadata argument.

The metadata arguments should work correctly without extra attributes like 'readonly' needed. And I can't swear the argument won't be changed to be stricter without touching the attributes. That would result in a miscompile, and it seems safer to just rely on the metadata argument and call it a day.


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:1072
+  // using the default environment!
+  if (!SimpleValue::canHandle(CondInst))
+    return false;
----------------
JosephTremoulet wrote:
> I'm confused why you need this, I think the caller is already checking it (see comment at callsite).
I wouldn't have done it without a reason, but I can't find a repro now. It might be related to further work and thus should be in a later ticket. I'll remove it.


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

https://reviews.llvm.org/D99962