[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend

mibintc added a comment. In D87528#2357970 , @pengfei wrote: > I agreed with Craig. Emitting constrained intrinsics on an unsupported target > may result in problems. It's better to check if it is supported and prevent > from the front end. Here's a

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend

uabelho added a comment. We got similar problems for our downstream target. We worked around that by bailing out early in PragmaSTDC_FENV_ACCESSHandler::HandlePragma for our target but perhaps one could check hasStrictFP() instead and ignore the pragma if that is false? Repository: rG LLVM

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend

pengfei added a comment. I agreed with Craig. Emitting constrained intrinsics on an unsupported target may result in problems. It's better to check if it is supported and prevent from the front end. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend

craig.topper added a comment. In D87528#2357043 , @mibintc wrote: > Actually kludging it by just removing the assert isn't going to work. I'll > ping Pengfei to see about developing a patch for this problem. This is likely a WebAssembly backend problem.

RE: [PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend

gt; Subject: [PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend > > dmajor added a comment. > > After this commit our bots have a failure on the wasm target, could you please > take a look? > > test.c: > > double a, b; > c() { > #pragma STDC FENV_ACCES

RE: [PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend

gmail.com; rich...@metafoo.co.uk; aaron.ball...@gmail.com > Cc: dma...@mozilla.com; dexonsm...@apple.com; sca...@apple.com; > kevin.n...@sas.com; cfe-commits@lists.llvm.org; mlek...@skidmore.edu; > blitzrak...@gmail.com; shen...@google.com; paul.robin...@sony.com > Subject: [PATCH] D87528: Enabl

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend

dmajor added a comment. After this commit our bots have a failure on the wasm target, could you please take a look? test.c: double a, b; c() { #pragma STDC FENV_ACCESS ON b == a; } Run `clang -cc1 -triple wasm32-unknown-wasi -emit-obj test.c` (clang needs to be build with

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG2e204e23911b: [clang] Enable support for #pragma STDC FENV_ACCESS (authored by mibintc). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend

mibintc updated this revision to Diff 300494. mibintc retitled this revision from "Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress" to "Enable '#pragma STDC FENV_ACCESS' in frontend". mibintc added a comment. I updated the test cases, added documentation about

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

sepavloff added inline comments. Comment at: clang/lib/Sema/SemaAttr.cpp:1020-1023 +// Resume the default rounding and exception modes. +NewFPFeatures.setRoundingModeOverride( +llvm::RoundingMode::NearestTiesToEven); +

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rsmith added inline comments. Comment at: clang/test/Parser/pragma-fenv_access.c:28 +#if defined(CPP) & defined(STRICT) +//not-expected-error@+3 {{constexpr variable 'frac' must be initialized by a constant expression}} +//not-expected-note@+2 {{compile time floating point

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc added inline comments. Comment at: clang/test/Parser/pragma-fenv_access.c:28 +#if defined(CPP) & defined(STRICT) +//not-expected-error@+3 {{constexpr variable 'frac' must be initialized by a constant expression}} +//not-expected-note@+2 {{compile time floating point

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc added inline comments. Comment at: clang/lib/Sema/SemaAttr.cpp:1020-1023 +// Resume the default rounding and exception modes. +NewFPFeatures.setRoundingModeOverride( +llvm::RoundingMode::NearestTiesToEven); +

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

sepavloff accepted this revision. sepavloff added a comment. LGTM There is concern with restoring FP options but it could be solved separately. Looking forward to results of SPEC runs! Comment at: clang/docs/UsersManual.rst:1389 * ``precise`` Disables optimizations that

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks, I'm happy with this, though please wait a day or two for comments from the other reviewers. We should also add some documentation covering the rules we're using for the

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc updated this revision to Diff 299426. mibintc added a comment. I made the change requested by @rsmith to HandleIntToFloatCast, avoiding the check if Info.InConstantContext, then I corrected 2 tests so that -verify matches. clang/test/CXX/expr/expr.const/p2-0x.cpp

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc marked 2 inline comments as done. mibintc added a comment. In D87528#2270541 , @rsmith wrote: >> @rsmith asked "I don't see changes to the constant evaluator". The semantic >> rules for enabling this pragma require that strict or precise

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc updated this revision to Diff 299395. mibintc added a comment. Herald added a reviewer: aaron.ballman. Herald added a subscriber: dexonsmith. I added StrictFPAttr clang function attribute accroding to @rjmccall 's suggestion instead of using a boolean on the function decl, rebased, and

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:2445 + FPO.getFPExceptionMode() != LangOptions::FPE_Ignore || + FPO.getAllowFEnvAccess()) && Info.Ctx.getLangOpts().CPlusPlus) { +Info.FFDiag(E,

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rsmith added a comment. FYI: I posted a patch to implement the "manifestly constant evaluated" rule as https://reviews.llvm.org/D89360. It looks like the stricter rule doesn't work in practice; far too much C++ code uses `-frounding-math` and expects to be able to still include things like

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rjmccall added a comment. The GCC docs seem pretty clear that `-frounding-math` is meant to allow dynamic access to the FP environment. There might be optimizations that are perfectly legal with a constant but non-default rounding mode, and there might be some flag that only implies that

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc added a comment. In D88498 @rsmith wrote, s far from clear to me that this is correct in C++. In principle, for a dynamic initializer, the rounding mode could have been set by an earlier initializer. Perhaps we can make an argument that, due to the

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc updated this revision to Diff 297260. mibintc added a comment. I rebased the patch I added documentation to UserManual showing that -ffp-model=strict enables FENV_ACCESS I removed ActOnAfterCompoundStatementLeadingPragmas since it was redundant with work being done in ActOnCompoundStmt

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc updated this revision to Diff 296520. mibintc added a comment. I added a sentence in the clang UserManual pointing out that ffp-model=strict automatically enables FENV_ACCESS. I changed checkFloatingPointResult the way @sepavloff suggested to solve the LIT failure in

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

sepavloff added a comment. In D87528#2312230 , @mibintc wrote: > For the LIT test clang/test/AST/const-fpfeatures.cpp, currently failing in > this patch, the variables V1 and others are initialized via call to "global > var init" which performs the

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc added a comment. For the LIT test clang/test/AST/const-fpfeatures.cpp, currently failing in this patch, the variables V1 and others are initialized via call to "global var init" which performs the rounding at execution time, I think that's correct not certain. Repository: rG LLVM

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc updated this revision to Diff 296221. mibintc added a comment. In the previous version of this patch, I enabled FENV_ACCESS if frounding-math, but @sepavloff commented that this deduction is not correct. I changed the logic to check for the "ffp-model=strict" settings, and if those

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:410-411 setRoundingMode(LO.getFPRoundingMode()); +// FENV access is true if rounding math is enabled. +setAllowFEnvAccess((getRoundingMode() == llvm::RoundingMode::Dynamic));

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc added a comment. I neglected to hit the "submit" button last week, these are the inline questions I have. Also in addition to the comments above, check-clang is showing 1 LIT test failing AST/const-fpfeatures.cpp I believe the clang constant folder is returning False for the

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

sepavloff added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:410-411 setRoundingMode(LO.getFPRoundingMode()); +// FENV access is true if rounding math is enabled. +setAllowFEnvAccess((getRoundingMode() == llvm::RoundingMode::Dynamic));

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc updated this revision to Diff 295880. mibintc added a comment. I patched my workspace with D88498 from Sept 29. I fixed some nits and also enabled FENV_ACCESS when the command line option specifies frounding-math. This corresponds to Microsoft's

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rjmccall added a comment. Richard and I had a long conversation about this further up-thread, if you missed it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87528/new/ https://reviews.llvm.org/D87528 ___ cfe-commits mailing list

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

sepavloff added a comment. In D87528#2299497 , @mibintc wrote: > In D87528#2297647 , @sepavloff wrote: > >> In D87528#2295015 , @mibintc wrote: >> >>> I tried using the 0924

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc added a comment. In D87528#2297647 , @sepavloff wrote: > In D87528#2295015 , @mibintc wrote: > >> I tried using the 0924 version of the patch on an internal workload SPEC >> "cpu2017" and found that a few

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:2446 + fpOptions.isFPConstrained()) { +Info.CCEDiag(E, diag::note_constexpr_float_arithmetic_strict); +return false; FFDiag Comment at:

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc added a comment. In D87528#2297647 , @sepavloff wrote: > In D87528#2295015 , @mibintc wrote: > >> I tried using the 0924 version of the patch on an internal workload SPEC >> "cpu2017" and found that a few

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

sepavloff added a comment. In D87528#2295015 , @mibintc wrote: > I tried using the 0924 version of the patch on an internal workload SPEC > "cpu2017" and found that a few files failed to compile because of an error > message on static initializer, like

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc added inline comments. Comment at: clang/test/CodeGen/fp-floatcontrol-pragma.cpp:154 + if (i<0) + return 1.0 + 2.0; + // Check that floating point constant folding doesn't occur if sepavloff wrote: > In this particular case we know for sure that the

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc added a comment. In D87528#2270502 , @sepavloff wrote: >> @sepavloff Is it OK if I continue work on this item? Not sure about the >> protocol when continuing someone else's patch. > > It is OK for me. There is also an action in Phabricator

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

sepavloff added inline comments. Comment at: clang/test/CodeGen/fp-floatcontrol-pragma.cpp:154 + if (i<0) + return 1.0 + 2.0; + // Check that floating point constant folding doesn't occur if In this particular case we know for sure that the result does not

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc marked 2 inline comments as done. mibintc added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:2683 } + if (const BinaryOperator *BE = dyn_cast(E)) { +if (BE->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained()) { rsmith

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc added a comment. Note, @sepavloff is working on overlapping concerns here, D87822 : [FPEnv] Evaluate constant expressions under non-default rounding modes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87528/new/ https://reviews.llvm.org/D87528

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc updated this revision to Diff 294074. mibintc added a comment. Responded to @rsmith's review CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87528/new/ https://reviews.llvm.org/D87528 Files: clang/include/clang/Basic/DiagnosticASTKinds.td

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:2683 } + if (const BinaryOperator *BE = dyn_cast(E)) { +if (BE->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained()) { `E` here is only intended for use in determining

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:13197-13198 assert(E->isRValue() && E->getType()->isRealFloatingType()); + if (Info.isStrictFP) +return false; return FloatExprEvaluator(Info, Result).Visit(E); rsmith wrote: >

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc updated this revision to Diff 292343. mibintc added a comment. I pulled out the isStrictFP boolean from EvalInfo and used the FPOptions when visiting floating point BinaryOperator, CastExpr and builtin CallExpr. I created the diagnostic note explaining why constant evaluation failed.

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rsmith requested changes to this revision. rsmith added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/AST/ExprConstant.cpp:775-778 +/// Strict floating point is enabled, this inhibits +/// floating ponit constant folding. +bool

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc updated this revision to Diff 291736. mibintc added a comment. This update uses context information from Expr->getFPFeaturesInEffect() to disable fp constant folding in ExprConstant.cpp Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rjmccall added a subscriber: scanon. rjmccall added a comment. That's a really useful concept, and given that it exists, I agree that we shouldn't invent something else. The fact that it covers local variables with constant initializers that happen to be `const` seems really unfortunate,

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rsmith added a comment. In D87528#2270561 , @rjmccall wrote: > Richard, what do you think the right rule is for when to use the special > constant-evaluation rules for this feature in C++? The C standard says that > constant expressions should use the

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rjmccall added a comment. Richard, what do you think the right rule is for when to use the special constant-evaluation rules for this feature in C++? The C standard says that constant expressions should use the default rules rather than considering the dynamic environment, but C can get away

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rsmith added a comment. > @rsmith asked "I don't see changes to the constant evaluator". The semantic > rules for enabling this pragma require that strict or precise semantics be in > effect., see SemaAttr.cpp And the floating point constant evaluation doesn't > occur when strict or precise

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

sepavloff added a comment. > @sepavloff Is it OK if I continue work on this item? Not sure about the > protocol when continuing someone else's patch. It is OK for me. There is also an action in Phabricator "Commandeer Revision" to transfer ownership on a revision item. I don't think however

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rjmccall added a comment. IRGen generally doesn't query the AST constant evaluator for arbitrary expressions; we do it for certain calls and loads because otherwise we might emit an illegal ODR-use, but otherwise we just expect LLVM's constant-folding to do a decent job. Repository: rG

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

mibintc created this revision. mibintc added reviewers: sepavloff, rjmccall. Herald added a project: clang. mibintc requested review of this revision. I rebased https://reviews.llvm.org/D69272 to work on the blocking comment from @rsmith @sepavloff Is it OK if I continue work on this item? Not