[PATCH] D88913: [FPEnv] Use strictfp metadata in casting nodes

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

kpn added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:618-619
     FPOptions OldFPFeatures;
+    llvm::fp::ExceptionBehavior OldExcept;
+    llvm::RoundingMode OldRounding;
     Optional<CGBuilderTy::FastMathFlagGuard> FMFGuard;
----------------
sepavloff wrote:
> Is it possible to merge this variables into `OldFPFeatures`? In what cases state of `Builder` may differ from the state of `CGF.CurFPFeatures`?
Without this patch they will differ if the AST changes CGF.CurFPFeatures and the Builder hasn't been updated yet. Say, because we're going back up the tree.

The concern is that we'll miss places that need to be updated to respect the AST. But if we don't make a point of resetting the Builder then we'll end up with stale data in the Builder that "just happens" to work. We need tests to actually test what they are supposed to test without relying on stale data.

The comments in the tests about "maytrap" being wrong show us that we absolutely have exactly this bug in multiple places. The Builder isn't getting properly updated and this change makes that visible and clear.


================
Comment at: clang/test/CodeGen/aarch64-v8.2a-neon-intrinsics-constrained.c:6
 // RUN: %clang_cc1 -triple arm64-none-linux-gnu -target-feature +neon -target-feature +fullfp16 -target-feature +v8.2a\
-// RUN: -ffp-exception-behavior=strict \
+// RUN: -ffp-exception-behavior=maytrap -DEXCEPT=1 \
 // RUN: -fexperimental-strict-floating-point \
----------------
sepavloff wrote:
> Why did you change exception behavior to `maytrap`?
Because "maytrap" is not the IRBuilder's default of "strict" and it isn't clang's default of "ignore".

The #pragma sets the exception handling to "strict". Having the command line arguments set a global "maytrap" with a #pragma that sets it to "strict" at the top of the file means that places in clang that need to be updated will become visible.


================
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:
> 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.


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

https://reviews.llvm.org/D88913