Re: [PATCH] D65994: Extended FPOptions with new attributes

On Wed, Mar 11, 2020 at 4:09 PM Serge Pavlov wrote: >In this environment, 5 bits are quite a lot for FP flexibility, and I >> think the complexity of reducing this is small and isolated (rounding + >> exceptions together fit in 4 bits) > > > Rounding (5 standard variants) and exception (3 var

Re: [PATCH] D65994: Extended FPOptions with new attributes

> >In this environment, 5 bits are quite a lot for FP flexibility, and I > think the complexity of reducing this is small and isolated (rounding + > exceptions together fit in 4 bits) Rounding (5 standard variants) and exception (3 variants) already do not fit 4 bits. And there is also precis

Re: [PATCH] D65994: Extended FPOptions with new attributes

On Wed, Mar 11, 2020 at 12:42 PM Serge Pavlov wrote: > It is a matter of taste, but for me it looks strange to develop complex > and error-prone system to save 7 bits at expense of readability and > maintainability. My observations show that clang AST consumes much less > memory than llvm objects

Re: [PATCH] D65994: Extended FPOptions with new attributes

It is a matter of taste, but for me it looks strange to develop complex and error-prone system to save 7 bits at expense of readability and maintainability. My observations show that clang AST consumes much less memory than llvm objects. FPOption could be shared between user using something like s

[PATCH] D65994: Extended FPOptions with new attributes

sammccall added a comment. This patch increased the used size of BinaryOperator by 5 bits. Those bits were all padding, but now BinaryOperatorBitfields is completely full at 32 bits and we can't add any new bits to Stmt without increasing BinaryOperator by 8 bytes. (See D75443

[PATCH] D65994: Extended FPOptions with new attributes

This revision was automatically updated to reflect the committed changes. Closed by commit rG4aea70ed3292: [FPEnv] Extended FPOptions with new attributes (authored by sepavloff). Changed prior to commit: https://reviews.llvm.org/D65994?vs=239893&id=240432#toc Repository: rG LLVM Github Monor

[PATCH] D65994: Extended FPOptions with new attributes

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, but I'll echo what @rjmccall said about this level of granularity being a bit too fine for review for future patches (we expect changes to be testable, which this is, but o

[PATCH] D65994: Extended FPOptions with new attributes

sepavloff updated this revision to Diff 239893. sepavloff added a comment. Rebased the patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65994/new/ https://reviews.llvm.org/D65994 Files: clang/include/clang/AST/Stmt.h clang/include/clang/Bas

[PATCH] D65994: Extended FPOptions with new attributes

rjmccall added a comment. Oh, sorry, I confused several patches, then. I don't actually think you need to break down the patches this finely; it would be fine to take all three steps to implement the feature in one pass. It's only important to break down the patches into more incremental compo

[PATCH] D65994: Extended FPOptions with new attributes

sepavloff marked 2 inline comments as done. sepavloff added a comment. In D65994#1625457 , @rjmccall wrote: > Since this setting changes IR output, you should write a test that emits IR > for source code and tests that you get the right IR output. This

[PATCH] D65994: Extended FPOptions with new attributes

sepavloff updated this revision to Diff 214754. sepavloff added a comment. Updated patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65994/new/ https://reviews.llvm.org/D65994 Files: clang/include/clang/AST/Stmt.h clang/include/clang/Basic/L

[PATCH] D65994: Extended FPOptions with new attributes

rjmccall added a comment. Since this setting changes IR output, you should write a test that emits IR for source code and tests that you get the right IR output. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65994/new/ https://reviews.llvm.org/D65

[PATCH] D65994: Extended FPOptions with new attributes

kpn added a comment. In D65994#1622840 , @aaron.ballman wrote: > In general, this seems reasonable, but is missing test code. The IRBuilder does have a strict FP mode setting now. When strict mode is enabled the (implemented) constrained intrinsics aut

[PATCH] D65994: Extended FPOptions with new attributes

aaron.ballman added a comment. In general, this seems reasonable, but is missing test code. Comment at: clang/include/clang/Basic/LangOptions.h:188 +/// Rounding to nearest, corresponds to "round.tonearest". +ToNearest, +/// Rounding toward -Inf, corresponds to "rou

[PATCH] D65994: Extended FPOptions with new attributes

sepavloff created this revision. sepavloff added reviewers: anemet, kpn, aaron.ballman, hfinkel, rsmith, rjmccall. Herald added a project: clang. This change added two new attributes, rounding mode and exception behavior to the structure FPOptions. These attributes allow more flexible treatment o