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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
