andrew.w.kaylor added inline comments.
================ Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3436 + } + } + ---------------- cameron.mcinally wrote: > andrew.w.kaylor wrote: > > cameron.mcinally wrote: > > > cameron.mcinally wrote: > > > > cameron.mcinally wrote: > > > > > I don't think it's safe to fuse a FMUL and FADD if the intermediate > > > > > rounding isn't exactly the same as those individual operations. > > > > > FMULADD doesn't guarantee that, does it? > > > > To be clear, we could miss very-edge-case overflow/underflow exceptions. > > > Ah, but I see C/C++ FP_CONTRACT allows the exceptions to be optimized > > > away. Sorry for the noise. > > We've talked about this before but I don't think we ever documented a > > decision as to whether we want to allow constrained intrinsics and fast > > math flags to be combined. This patch moves that decision into clang's > > decision to generate this intrinsic or not. > > > > I think it definitely makes sense in the case of fp contraction, because > > even if a user cares about value safety they might want FMA, which is > > theorectically more accurate than the separate values even though it > > produces a different value. This is consistent with gcc (which produces FMA > > under "-ffp-contract=fast -fno-fast-math") and icc (which produced FMA > > under "-fp-model strict -fma"). > > > > For the record, I also think it makes sense to use nnan, ninf, and nsz with > > constrained intrinsics. > You had me until: > > >For the record, I also think it makes sense to use nnan, ninf, and nsz with > >constrained intrinsics. > > To be clear, we'd need them for the `fast` case, but I don't see a lot of > value for the `strict` case. > > We definitely want reassoc/recip/etc for the `optimized but trap-safe` case, > so that's enough to require FMF flags on constrained intrinsics alone. > > We should probably break this conversation out into an llvm-dev thread... I agree about starting an llvm-dev thread. I'll send something out unless you've already done so by the time I finish typing it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72820/new/ https://reviews.llvm.org/D72820 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
