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

sepavloff accepted this revision. sepavloff added a comment. This revision is now accepted and ready to land. LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88913/new/ https://reviews.llvm.org/D88913 ___ cfe-commits mailing list cfe-comm

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

kpn added a comment. Update for review comments. Comment at: clang/test/CodeGen/builtin_float_strictfp.c:1 +// RUN: %clang_cc1 -emit-llvm -triple x86_64-windows-pc -ffp-exception-behavior=maytrap -o - %s | FileCheck %s --check-prefixes=CHECK,FP16 +// RUN: %clang_cc1 -emit-llv

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

kpn added a comment. I'll see how much I can simplify of the tests. Comment at: clang/test/CodeGen/exprs-strictfp.c:1 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -emit-llvm -o - | FileCheck %s +// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -ffp-exception-beha

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

sepavloff added a comment. This patch contains many tests and they are valuable. However many of them are likely to be compatible with unpatched compiler, so are not related to casting nodes. I would propose to move such tests into a separate patch and make this one dependent on it. Leave here

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

sepavloff added a comment. In D88913#2356152 , @kpn wrote: > In D88913#2353379 , @sepavloff wrote: > >> Generally the patch looks good. But the need to expect incorrect values in >> tests is a concern. Maybe this i

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

kpn added a comment. In 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 CGFPOptionsRA

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

sepavloff added a comment. 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. Comment at: clang/test

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

kpn added inline comments. 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. + kpn wrote:

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

kpn added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:154-155 + OldRounding = CGF.Builder.getDefaultConstrainedRounding(); if (OldFPFeatures == FPFeatures) return; sepavloff wrote: > This check can be lifted to just after the def

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

sepavloff added inline comments. 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. + kpn

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

kpn added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:618-619 FPOptions OldFPFeatures; +llvm::fp::ExceptionBehavior OldExcept; +llvm::RoundingMode OldRounding; Optional FMFGuard; sepavloff wrote: > Is it possible to merge th

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

sepavloff added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:137-139 + FPOptions FPFeatures = E->getFPFeaturesInEffect(CGF.getLangOpts()); + + ConstructorHelper(FPFeatures); The local variable `FPFeatures` can be eliminated here. ===

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

kpn added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:139 + + switch (E->getStmtClass()) { + case Stmt::UnaryOperatorClass: { sepavloff wrote: > Actually this is not correct, because subclass relations are not preserved in > this case. F

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

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:700 Res = ImplicitCastExpr::Create(Context, T, CK, E, nullptr, VK_RValue, - FPOptionsOverride()); + CurFPFeatureOverrides()); -

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

sepavloff added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:139 + + switch (E->getStmtClass()) { + case Stmt::UnaryOperatorClass: { Actually this is not correct, because subclass relations are not preserved in this case. For instance, `C