kpn added a comment. In D88913#2353379 <https://reviews.llvm.org/D88913#2353379>, @sepavloff wrote:
> Generally the patch looks good. But the need to expect incorrect values in > tests is a concern. Maybe this is a consequence of storing exception behavior > in a separate field of CGFPOptionsRAII. This misbehavior should be fixed. In this patch? Because that's going to be a huge patch. Fixing all the issues with strictfp AST->IRBuilder is going to be large. Fixing them incrementally seems better to me. Eliminating the incorrect values in the tests marked FIXME would sweep the problem under the rug. It's true that incorrect values in tests is _not_ the desired end state. But it seems to me that as a _temporary_ incremental step it beats the alternatives. ================ Comment at: clang/test/CodeGen/aarch64-v8.2a-neon-intrinsics-constrained.c:26 +// metadata from the AST instead of the global default from the command line. +// FIXME: All cases of "fpexcept.maytrap" in this test are wrong. + ---------------- sepavloff wrote: > kpn wrote: > > kpn wrote: > > > sepavloff wrote: > > > > kpn wrote: > > > > > sepavloff wrote: > > > > > > Why they are wrong? > > > > > Because the #pragma covers the entire file and sets exception > > > > > handling to "strict". Thus all constrained intrinsic calls should be > > > > > "strict", and if they are "maytrap" or "ignore" then we have a bug. > > > > What is the reason for that? Does `#pragma float_control` work > > > > incorrectly? Why in `clang/test/CodeGen/complex-math-strictfp.c` > > > > exception handling is correct? > > > The #pragma works just fine. The problem is that we need to get the > > > strictfp bits from the AST to the IRBuilder, and we haven't finished > > > implementing that. So sometimes it works, like in > > > complex-math-strictfp.c, and sometimes it doesn't. As you can see in the > > > tests in this patch. > > I forgot to mention that complex-math-strictfp.c gets a correct BinOpInfo > > passed down to get the IRBuilder set correctly. But there are other places > > that don't correctly set BinOpInfo and so we get inconsistent behavior > > despite the call to the IRBuilder being wrapped in FPOptsRAII. And _that's_ > > why I structured this patch the way I did. > > The problem is that we need to get the strictfp bits from the AST to the > > IRBuilder > > What is the status of this problem? Is it on track? This is a larger problem that we need to tackle incrementally. I'd like to get a signoff on the approach taken in this patch before going too far down the road of writing fixes for all the issues we have here. But it is at the top of my list. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88913/new/ https://reviews.llvm.org/D88913 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
