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

[Sam McCall via cfe-commits]

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 variants) already do not
> fit 4 bits.
>
Sure they do: there are 5 x 3 = 15 combinations, which can be encoded in 4
bits. This would be a short-term fix only as it sounds like the number of
bits will grow further.

Fortunately Melanie is working on a better/more scalable solution of
putting the FPOptions in trailing objects of the nodes where they're
actually used (e.g. most BinaryOperators aren't floating point at all).
Then FPOptions can grow as many bits as needed without affecting many nodes.


> And there is also precision, denormals treatment, exception mask, they are
> usually represented in hardware register and
> make up a floating point environment. Instead of putting all these bits to
> different places it would be better to collect them in one place, it could
> make operations simpler and faster.
>
>   both rounding and exceptions seem essentially unused at present.
>
>
> Implementation of advanced floating point support is in progress, they
> will be used more and more.
>
That sounds great! Sorry, I didn't mean to imply at all they were useless,
just as an uninformed person I can't reason about them by looking at uses
as they don't exist yet.

  (Note that the cost here is not 7 bits per node but 63 - once the bits
>> available in Stmt are full the stmt subclass needs to gain a member, and
>> its alignment is 8 *bytes*)
>
>
> As objects are allocated aligned, if size in not multiple of the
> alignment, some part of memory is spent uselessly anyway.
>
Right. My point is that today, BinaryOperator uses exactly all its bits.
Tomorrow, we need to add a HasError bit to Expr. So tomorrow, the marginal
cost of the last bit (or savings from squeezing the last bit) is 64 bits,
at least for BinOp. This is certainly a short-term view, the long-term
option is probably moving FPOptions out of the bitfields in some way.

Use cases vary, and many tools deal with large ASTs but don't use LLVM
>> codegen at all.
>
>
> That is true, but developer tools are usually less restricted in computing
> resources. Besides hard economy in memory often means some loss of
> performance.
>
I'm not sure this is productive - if you don't think bit-packing is a
sensible tradeoff, then I think to some extent it's on you to change it,
rather than eat all the bits and walk away! I do happen to work on
developer tools with memory constraints, but that's not the hat I'm wearing
today - we're just stuck between the established design (which aims for
compactness) and existing uneconomical use of it.

FPOption could be shared between user using something like shared_ptr, but
>>> it means expenses of 64 bit for a pointer. Don't know if it makes sense...
>>> As you say, I don't think this saves anything, unless we can stop
>>> storing the pointer in BinaryOperator.
>>
>>
> We could keep pointer to shared properties, thus saving memory. Moreover
> such facility could enable some techniques using this shared property as a
> marker. For example, all floating point operations obtained from the same
> region where `pragma STDC FENV_ACCESS` is in act could point to the same
> FPOption object, thus we could distinguish between different regions. It
> would save memory consumption without sacrifice of maintainability and
> speed.
>
Yeah, just concretely needing an 8-byte pointer to a 1-byte shared
structure doesn't save memory, so some larger refactoring/design would be
needed. TrailingObjects is simpler.


> Thanks,
> --Serge
>
>
> On Wed, Mar 11, 2020 at 8:01 PM Sam McCall  wrote:
>
>> 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.
>>>
>> I agree, and would be happy if we had a design without these scarce bits,
>> but AFAICS we don't. 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). But I ask about
>> the domain because maybe there's a simple but smaller model I can't really
>> infer this myself, both rounding and exceptions seem essentially unused at
>> present.
>> (Note that the cost here is not 7 bits per node but 63 - once the bits
>> available in Stmt are full the stmt subclass needs to gain a member, and
>> its alignment is 8 *bytes*)
>>
>> Use cases vary, and many tools deal with large ASTs but don't use LLVM
>> codegen at all.
>>
>> FPOption could be shared between user using something like shared_ptr,
>>> but it means expenses of 64 bit for a 

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

[Serge Pavlov via cfe-commits]

>
>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 precision, denormals treatment, exception
mask, they are usually represented in hardware register and
make up a floating point environment. Instead of putting all these bits to
different places it would be better to collect them in one place, it could
make operations simpler and faster.

  both rounding and exceptions seem essentially unused at present.


Implementation of advanced floating point support is in progress, they will
be used more and more.

  (Note that the cost here is not 7 bits per node but 63 - once the bits
> available in Stmt are full the stmt subclass needs to gain a member, and
> its alignment is 8 *bytes*)


As objects are allocated aligned, if size in not multiple of the alignment,
some part of memory is spent uselessly anyway.

Use cases vary, and many tools deal with large ASTs but don't use LLVM
> codegen at all.


That is true, but developer tools are usually less restricted in computing
resources. Besides hard economy in memory often means some loss of
performance.

FPOption could be shared between user using something like shared_ptr, but
>> it means expenses of 64 bit for a pointer. Don't know if it makes sense...
>> As you say, I don't think this saves anything, unless we can stop storing
>> the pointer in BinaryOperator.
>
>
We could keep pointer to shared properties, thus saving memory. Moreover
such facility could enable some techniques using this shared property as a
marker. For example, all floating point operations obtained from the same
region where `pragma STDC FENV_ACCESS` is in act could point to the same
FPOption object, thus we could distinguish between different regions. It
would save memory consumption without sacrifice of maintainability and
speed.

Thanks,
--Serge


On Wed, Mar 11, 2020 at 8:01 PM Sam McCall  wrote:

> 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.
>>
> I agree, and would be happy if we had a design without these scarce bits,
> but AFAICS we don't. 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). But I ask about
> the domain because maybe there's a simple but smaller model I can't really
> infer this myself, both rounding and exceptions seem essentially unused at
> present.
> (Note that the cost here is not 7 bits per node but 63 - once the bits
> available in Stmt are full the stmt subclass needs to gain a member, and
> its alignment is 8 *bytes*)
>
> Use cases vary, and many tools deal with large ASTs but don't use LLVM
> codegen at all.
>
> FPOption could be shared between user using something like shared_ptr, but
>> it means expenses of 64 bit for a pointer. Don't know if it makes sense...
>>
> As you say, I don't think this saves anything, unless we can stop storing
> the pointer in BinaryOperator.
>
> Cheers, Sam
>
>
>>
>>
>> Thanks,
>> --Serge
>>
>>
>> On Wed, Mar 11, 2020 at 6:30 PM Sam McCall via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> 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 <
>>> https://reviews.llvm.org/D75443> and D54526 <
>>> https://reviews.llvm.org/D54526> for the optimization this would
>>> revert).
>>>
>>> To squeeze in the new bit I'm planning to suggest squeezing getInt() to
>>> 7 bits (it encodes 3x2x5x3 = 90 states, so this is possible) but I'm not
>>> really familiar with this domain - if many of the 90 states are not
>>> possible it'd probably be useful to have some more bits back :-)
>>>
>>>
>>> Repository:
>>>   rG LLVM Github Monorepo
>>>
>>> CHANGES SINCE LAST ACTION
>>>   https://reviews.llvm.org/D65994/new/
>>>
>>> https://reviews.llvm.org/D65994
>>>
>>>
>>>
>>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via cfe-commits]

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.
>
I agree, and would be happy if we had a design without these scarce bits,
but AFAICS we don't. 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). But I ask about
the domain because maybe there's a simple but smaller model I can't really
infer this myself, both rounding and exceptions seem essentially unused at
present.
(Note that the cost here is not 7 bits per node but 63 - once the bits
available in Stmt are full the stmt subclass needs to gain a member, and
its alignment is 8 *bytes*)

Use cases vary, and many tools deal with large ASTs but don't use LLVM
codegen at all.

FPOption could be shared between user using something like shared_ptr, but
> it means expenses of 64 bit for a pointer. Don't know if it makes sense...
>
As you say, I don't think this saves anything, unless we can stop storing
the pointer in BinaryOperator.

Cheers, Sam


>
>
> Thanks,
> --Serge
>
>
> On Wed, Mar 11, 2020 at 6:30 PM Sam McCall via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> 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 <
>> https://reviews.llvm.org/D75443> and D54526 <
>> https://reviews.llvm.org/D54526> for the optimization this would revert).
>>
>> To squeeze in the new bit I'm planning to suggest squeezing getInt() to 7
>> bits (it encodes 3x2x5x3 = 90 states, so this is possible) but I'm not
>> really familiar with this domain - if many of the 90 states are not
>> possible it'd probably be useful to have some more bits back :-)
>>
>>
>> Repository:
>>   rG LLVM Github Monorepo
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D65994/new/
>>
>> https://reviews.llvm.org/D65994
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Serge Pavlov via cfe-commits]

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 shared_ptr, but
it means expenses of 64 bit for a pointer. Don't know if it makes sense...


Thanks,
--Serge


On Wed, Mar 11, 2020 at 6:30 PM Sam McCall via Phabricator <
revi...@reviews.llvm.org> wrote:

> 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 
> and D54526  for the optimization this
> would revert).
>
> To squeeze in the new bit I'm planning to suggest squeezing getInt() to 7
> bits (it encodes 3x2x5x3 = 90 states, so this is possible) but I'm not
> really familiar with this domain - if many of the 90 states are not
> possible it'd probably be useful to have some more bits back :-)
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D65994/new/
>
> https://reviews.llvm.org/D65994
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65994: Extended FPOptions with new attributes

[Sam McCall via Phabricator via cfe-commits]

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  and 
D54526  for the optimization this would 
revert).

To squeeze in the new bit I'm planning to suggest squeezing getInt() to 7 bits 
(it encodes 3x2x5x3 = 90 states, so this is possible) but I'm not really 
familiar with this domain - if many of the 90 states are not possible it'd 
probably be useful to have some more bits back :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65994



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65994: Extended FPOptions with new attributes

[Serge Pavlov via Phabricator via cfe-commits]

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=240432#toc

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/LangOptions.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/TreeTransform.h

Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -9938,7 +9938,7 @@
   RHS.get() == E->getRHS())
 return E;
 
-  Sema::FPContractStateRAII FPContractState(getSema());
+  Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
   getSema().FPFeatures = E->getFPFeatures();
 
   return getDerived().RebuildBinaryOperator(E->getOperatorLoc(), E->getOpcode(),
@@ -10464,7 +10464,7 @@
   (E->getNumArgs() != 2 || Second.get() == E->getArg(1)))
 return SemaRef.MaybeBindToTemporary(E);
 
-  Sema::FPContractStateRAII FPContractState(getSema());
+  Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
   getSema().FPFeatures = E->getFPFeatures();
 
   return getDerived().RebuildCXXOperatorCallExpr(E->getOperator(),
Index: clang/lib/Sema/SemaAttr.cpp
===
--- clang/lib/Sema/SemaAttr.cpp
+++ clang/lib/Sema/SemaAttr.cpp
@@ -940,6 +940,14 @@
   }
 }
 
+void Sema::setRoundingMode(LangOptions::FPRoundingModeKind FPR) {
+  FPFeatures.setRoundingMode(FPR);
+}
+
+void Sema::setExceptionMode(LangOptions::FPExceptionModeKind FPE) {
+  FPFeatures.setExceptionMode(FPE);
+}
+
 void Sema::ActOnPragmaFEnvAccess(LangOptions::FEnvAccessModeKind FPC) {
   switch (FPC) {
   case LangOptions::FEA_On:
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1014,9 +1014,9 @@
 Tok.getLocation(),
 "in compound statement ('{}')");
 
-  // Record the state of the FP_CONTRACT pragma, restore on leaving the
+  // Record the state of the FPFeatures, restore on leaving the
   // compound statement.
-  Sema::FPContractStateRAII SaveFPContractState(Actions);
+  Sema::FPFeaturesStateRAII SaveFPContractState(Actions);
 
   InMessageExpressionRAIIObject InMessage(*this, false);
   BalancedDelimiterTracker T(*this, tok::l_brace);
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1306,12 +1306,12 @@
   /// should not be used elsewhere.
   void EmitCurrentDiagnostic(unsigned DiagID);
 
-  /// Records and restores the FP_CONTRACT state on entry/exit of compound
+  /// Records and restores the FPFeatures state on entry/exit of compound
   /// statements.
-  class FPContractStateRAII {
+  class FPFeaturesStateRAII {
   public:
-FPContractStateRAII(Sema ) : S(S), OldFPFeaturesState(S.FPFeatures) {}
-~FPContractStateRAII() { S.FPFeatures = OldFPFeaturesState; }
+FPFeaturesStateRAII(Sema ) : S(S), OldFPFeaturesState(S.FPFeatures) {}
+~FPFeaturesStateRAII() { S.FPFeatures = OldFPFeaturesState; }
 
   private:
 Sema& S;
@@ -9409,6 +9409,12 @@
   /// \#pragma STDC FENV_ACCESS
   void ActOnPragmaFEnvAccess(LangOptions::FEnvAccessModeKind FPC);
 
+  /// Called to set rounding mode for floating point operations.
+  void setRoundingMode(LangOptions::FPRoundingModeKind);
+
+  /// Called to set exception behavior for floating point operations.
+  void setExceptionMode(LangOptions::FPExceptionModeKind);
+
   /// AddAlignmentAttributesForRecord - Adds any needed alignment attributes to
   /// a the record decl, to handle '\#pragma pack' and '\#pragma options align'.
   void AddAlignmentAttributesForRecord(RecordDecl *RD);
Index: clang/include/clang/Basic/LangOptions.h
===
--- clang/include/clang/Basic/LangOptions.h
+++ clang/include/clang/Basic/LangOptions.h
@@ -357,17 +357,25 @@
 class FPOptions {
 public:
   FPOptions() : fp_contract(LangOptions::FPC_Off),
-fenv_access(LangOptions::FEA_Off) {}
+fenv_access(LangOptions::FEA_Off),
+rounding(LangOptions::FPR_ToNearest),
+exceptions(LangOptions::FPE_Ignore)
+{}
 
   // Used for serializing.
   explicit FPOptions(unsigned I)
   : fp_contract(static_cast(I & 3)),
-fenv_access(static_cast((I >> 2) & 1))
+fenv_access(static_cast((I >> 2) & 1)),
+rounding(static_cast((I >> 3) & 7)),
+exceptions(static_cast((I >> 6) & 3))
  

[PATCH] D65994: Extended FPOptions with new attributes

[Aaron Ballman via Phabricator via cfe-commits]

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 only if you also review other patches and notice the tests 
and how they relate to this patch, which can be easy for reviewers to miss).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65994



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65994: Extended FPOptions with new attributes

[Serge Pavlov via Phabricator via cfe-commits]

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/Basic/LangOptions.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/TreeTransform.h

Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -9938,7 +9938,7 @@
   RHS.get() == E->getRHS())
 return E;
 
-  Sema::FPContractStateRAII FPContractState(getSema());
+  Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
   getSema().FPFeatures = E->getFPFeatures();
 
   return getDerived().RebuildBinaryOperator(E->getOperatorLoc(), E->getOpcode(),
@@ -10464,7 +10464,7 @@
   (E->getNumArgs() != 2 || Second.get() == E->getArg(1)))
 return SemaRef.MaybeBindToTemporary(E);
 
-  Sema::FPContractStateRAII FPContractState(getSema());
+  Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
   getSema().FPFeatures = E->getFPFeatures();
 
   return getDerived().RebuildCXXOperatorCallExpr(E->getOperator(),
Index: clang/lib/Sema/SemaAttr.cpp
===
--- clang/lib/Sema/SemaAttr.cpp
+++ clang/lib/Sema/SemaAttr.cpp
@@ -940,6 +940,14 @@
   }
 }
 
+void Sema::setRoundingMode(LangOptions::FPRoundingModeKind FPR) {
+  FPFeatures.setRoundingMode(FPR);
+}
+
+void Sema::setExceptionMode(LangOptions::FPExceptionModeKind FPE) {
+  FPFeatures.setExceptionMode(FPE);
+}
+
 void Sema::ActOnPragmaFEnvAccess(LangOptions::FEnvAccessModeKind FPC) {
   switch (FPC) {
   case LangOptions::FEA_On:
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1014,9 +1014,9 @@
 Tok.getLocation(),
 "in compound statement ('{}')");
 
-  // Record the state of the FP_CONTRACT pragma, restore on leaving the
+  // Record the state of the FPFeatures, restore on leaving the
   // compound statement.
-  Sema::FPContractStateRAII SaveFPContractState(Actions);
+  Sema::FPFeaturesStateRAII SaveFPContractState(Actions);
 
   InMessageExpressionRAIIObject InMessage(*this, false);
   BalancedDelimiterTracker T(*this, tok::l_brace);
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1295,12 +1295,12 @@
   /// should not be used elsewhere.
   void EmitCurrentDiagnostic(unsigned DiagID);
 
-  /// Records and restores the FP_CONTRACT state on entry/exit of compound
+  /// Records and restores the FPFeatures state on entry/exit of compound
   /// statements.
-  class FPContractStateRAII {
+  class FPFeaturesStateRAII {
   public:
-FPContractStateRAII(Sema ) : S(S), OldFPFeaturesState(S.FPFeatures) {}
-~FPContractStateRAII() { S.FPFeatures = OldFPFeaturesState; }
+FPFeaturesStateRAII(Sema ) : S(S), OldFPFeaturesState(S.FPFeatures) {}
+~FPFeaturesStateRAII() { S.FPFeatures = OldFPFeaturesState; }
 
   private:
 Sema& S;
@@ -9385,6 +9385,12 @@
   /// \#pragma STDC FENV_ACCESS
   void ActOnPragmaFEnvAccess(LangOptions::FEnvAccessModeKind FPC);
 
+  /// Called to set rounding mode for floating point operations.
+  void setRoundingMode(LangOptions::FPRoundingModeKind);
+
+  /// Called to set exception behavior for floating point operations.
+  void setExceptionMode(LangOptions::FPExceptionModeKind);
+
   /// AddAlignmentAttributesForRecord - Adds any needed alignment attributes to
   /// a the record decl, to handle '\#pragma pack' and '\#pragma options align'.
   void AddAlignmentAttributesForRecord(RecordDecl *RD);
Index: clang/include/clang/Basic/LangOptions.h
===
--- clang/include/clang/Basic/LangOptions.h
+++ clang/include/clang/Basic/LangOptions.h
@@ -357,17 +357,25 @@
 class FPOptions {
 public:
   FPOptions() : fp_contract(LangOptions::FPC_Off),
-fenv_access(LangOptions::FEA_Off) {}
+fenv_access(LangOptions::FEA_Off),
+rounding(LangOptions::FPR_ToNearest),
+exceptions(LangOptions::FPE_Ignore)
+{}
 
   // Used for serializing.
   explicit FPOptions(unsigned I)
   : fp_contract(static_cast(I & 3)),
-fenv_access(static_cast((I >> 2) & 1))
+fenv_access(static_cast((I >> 2) & 1)),
+rounding(static_cast((I >> 3) & 7)),
+exceptions(static_cast((I >> 6) & 3))
 {}
 
   explicit FPOptions(const LangOptions )
   : fp_contract(LangOpts.getDefaultFPContractMode()),
-fenv_access(LangOptions::FEA_Off) {}
+

[PATCH] D65994: Extended FPOptions with new attributes

[John McCall via Phabricator via cfe-commits]

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 components if 
there's significant refactoring required to prepare for the patch or if each 
step is substantially complex on its own.  And it's nice for feature work to 
always be testable *somehow*.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65994



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65994: Extended FPOptions with new attributes

[Serge Pavlov via Phabricator via cfe-commits]

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 is a part of patch chain, it extends FPOptions with new options. In 
D65997  `pragma clang fp` is extended to 
manipulate these options. Finally D66092  
modifies code generator so that it emit IR depending on the new options in 
FPOptions, it makes possible to write IR tests.

In D65994#1623088 , @kpn wrote:

> 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 automatically replace the 
> normal FP instructions. I wonder if that would be right for testing of this 
> patch?


It is just what D66092  does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65994



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65994: Extended FPOptions with new attributes

[Serge Pavlov via Phabricator via cfe-commits]

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/LangOptions.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/TreeTransform.h

Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -9698,7 +9698,7 @@
   RHS.get() == E->getRHS())
 return E;
 
-  Sema::FPContractStateRAII FPContractState(getSema());
+  Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
   getSema().FPFeatures = E->getFPFeatures();
 
   return getDerived().RebuildBinaryOperator(E->getOperatorLoc(), E->getOpcode(),
@@ -10180,7 +10180,7 @@
   (E->getNumArgs() != 2 || Second.get() == E->getArg(1)))
 return SemaRef.MaybeBindToTemporary(E);
 
-  Sema::FPContractStateRAII FPContractState(getSema());
+  Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
   getSema().FPFeatures = E->getFPFeatures();
 
   return getDerived().RebuildCXXOperatorCallExpr(E->getOperator(),
Index: clang/lib/Sema/SemaAttr.cpp
===
--- clang/lib/Sema/SemaAttr.cpp
+++ clang/lib/Sema/SemaAttr.cpp
@@ -935,6 +935,14 @@
   }
 }
 
+void Sema::setRoundingMode(LangOptions::FPRoundingModeKind FPR) {
+  FPFeatures.setRoundingMode(FPR);
+}
+
+void Sema::setExceptionMode(LangOptions::FPExceptionModeKind FPE) {
+  FPFeatures.setExceptionMode(FPE);
+}
+
 void Sema::ActOnPragmaFEnvAccess(LangOptions::FEnvAccessModeKind FPC) {
   switch (FPC) {
   case LangOptions::FEA_On:
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -998,9 +998,9 @@
 Tok.getLocation(),
 "in compound statement ('{}')");
 
-  // Record the state of the FP_CONTRACT pragma, restore on leaving the
+  // Record the state of the FPFeatures, restore on leaving the
   // compound statement.
-  Sema::FPContractStateRAII SaveFPContractState(Actions);
+  Sema::FPFeaturesStateRAII SaveFPContractState(Actions);
 
   InMessageExpressionRAIIObject InMessage(*this, false);
   BalancedDelimiterTracker T(*this, tok::l_brace);
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1258,12 +1258,12 @@
   /// should not be used elsewhere.
   void EmitCurrentDiagnostic(unsigned DiagID);
 
-  /// Records and restores the FP_CONTRACT state on entry/exit of compound
+  /// Records and restores the FPFeatures state on entry/exit of compound
   /// statements.
-  class FPContractStateRAII {
+  class FPFeaturesStateRAII {
   public:
-FPContractStateRAII(Sema ) : S(S), OldFPFeaturesState(S.FPFeatures) {}
-~FPContractStateRAII() { S.FPFeatures = OldFPFeaturesState; }
+FPFeaturesStateRAII(Sema ) : S(S), OldFPFeaturesState(S.FPFeatures) {}
+~FPFeaturesStateRAII() { S.FPFeatures = OldFPFeaturesState; }
 
   private:
 Sema& S;
@@ -8759,6 +8759,12 @@
   /// \#pragma STDC FENV_ACCESS
   void ActOnPragmaFEnvAccess(LangOptions::FEnvAccessModeKind FPC);
 
+  /// Called to set rounding mode for floating point operations.
+  void setRoundingMode(LangOptions::FPRoundingModeKind);
+
+  /// Called to set exception behavior for floating point operations.
+  void setExceptionMode(LangOptions::FPExceptionModeKind);
+
   /// AddAlignmentAttributesForRecord - Adds any needed alignment attributes to
   /// a the record decl, to handle '\#pragma pack' and '\#pragma options align'.
   void AddAlignmentAttributesForRecord(RecordDecl *RD);
Index: clang/include/clang/Basic/LangOptions.h
===
--- clang/include/clang/Basic/LangOptions.h
+++ clang/include/clang/Basic/LangOptions.h
@@ -178,6 +178,34 @@
 FEA_On
   };
 
+  // Values of the following enumerations correspond to metadata arguments
+  // specified for constrained floating-point intrinsics:
+  // http://llvm.org/docs/LangRef.html#constrained-floating-point-intrinsics.
+
+  /// Possible rounding modes.
+  enum FPRoundingModeKind {
+/// Rounding to nearest, corresponds to "round.tonearest".
+FPR_ToNearest,
+/// Rounding toward -Inf, corresponds to "round.downward".
+FPR_Downward,
+/// Rounding toward +Inf, corresponds to "round.upward".
+FPR_Upward,
+/// Rounding toward zero, corresponds to "round.towardzero".
+FPR_TowardZero,
+/// Is determined by runtime environment, corresponds to "round.dynamic".
+FPR_Dynamic
+  };
+
+  /// Possible floating point exception 

[PATCH] D65994: Extended FPOptions with new attributes

[John McCall via Phabricator via cfe-commits]

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/D65994



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65994: Extended FPOptions with new attributes

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

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 automatically replace the 
normal FP instructions. I wonder if that would be right for testing of this 
patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65994



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65994: Extended FPOptions with new attributes

[Aaron Ballman via Phabricator via cfe-commits]

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 "round.downward".

Can you give these an `FPR` prefix?



Comment at: clang/include/clang/Basic/LangOptions.h:202
+/// Assume that floating-point exceptions are masked.
+Ignore,
+/// Transformations do not cause new exceptions but may hide some.

And these an `FPE` prefix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65994



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65994: Extended FPOptions with new attributes

[Serge Pavlov via Phabricator via cfe-commits]

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 of specific floating point environment than it is
provided by fenv_access.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65994

Files:
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/TreeTransform.h

Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -9698,7 +9698,7 @@
   RHS.get() == E->getRHS())
 return E;
 
-  Sema::FPContractStateRAII FPContractState(getSema());
+  Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
   getSema().FPFeatures = E->getFPFeatures();
 
   return getDerived().RebuildBinaryOperator(E->getOperatorLoc(), E->getOpcode(),
@@ -10180,7 +10180,7 @@
   (E->getNumArgs() != 2 || Second.get() == E->getArg(1)))
 return SemaRef.MaybeBindToTemporary(E);
 
-  Sema::FPContractStateRAII FPContractState(getSema());
+  Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
   getSema().FPFeatures = E->getFPFeatures();
 
   return getDerived().RebuildCXXOperatorCallExpr(E->getOperator(),
Index: clang/lib/Sema/SemaAttr.cpp
===
--- clang/lib/Sema/SemaAttr.cpp
+++ clang/lib/Sema/SemaAttr.cpp
@@ -935,6 +935,14 @@
   }
 }
 
+void Sema::setRoundingMode(LangOptions::FPRoundingModeKind FPR) {
+  FPFeatures.setRoundingMode(FPR);
+}
+
+void Sema::setExceptionMode(LangOptions::FPExceptionModeKind FPE) {
+  FPFeatures.setExceptionMode(FPE);
+}
+
 void Sema::ActOnPragmaFEnvAccess(LangOptions::FEnvAccessModeKind FPC) {
   switch (FPC) {
   case LangOptions::FEA_On:
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -998,9 +998,9 @@
 Tok.getLocation(),
 "in compound statement ('{}')");
 
-  // Record the state of the FP_CONTRACT pragma, restore on leaving the
+  // Record the state of the FPFeatures, restore on leaving the
   // compound statement.
-  Sema::FPContractStateRAII SaveFPContractState(Actions);
+  Sema::FPFeaturesStateRAII SaveFPContractState(Actions);
 
   InMessageExpressionRAIIObject InMessage(*this, false);
   BalancedDelimiterTracker T(*this, tok::l_brace);
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1258,12 +1258,12 @@
   /// should not be used elsewhere.
   void EmitCurrentDiagnostic(unsigned DiagID);
 
-  /// Records and restores the FP_CONTRACT state on entry/exit of compound
+  /// Records and restores the FPFeatures state on entry/exit of compound
   /// statements.
-  class FPContractStateRAII {
+  class FPFeaturesStateRAII {
   public:
-FPContractStateRAII(Sema ) : S(S), OldFPFeaturesState(S.FPFeatures) {}
-~FPContractStateRAII() { S.FPFeatures = OldFPFeaturesState; }
+FPFeaturesStateRAII(Sema ) : S(S), OldFPFeaturesState(S.FPFeatures) {}
+~FPFeaturesStateRAII() { S.FPFeatures = OldFPFeaturesState; }
 
   private:
 Sema& S;
@@ -8759,6 +8759,12 @@
   /// \#pragma STDC FENV_ACCESS
   void ActOnPragmaFEnvAccess(LangOptions::FEnvAccessModeKind FPC);
 
+  /// Called to set rounding mode for floating point operations.
+  void setRoundingMode(LangOptions::FPRoundingModeKind);
+
+  /// Called to set exception behavior for floating point operations.
+  void setExceptionMode(LangOptions::FPExceptionModeKind);
+
   /// AddAlignmentAttributesForRecord - Adds any needed alignment attributes to
   /// a the record decl, to handle '\#pragma pack' and '\#pragma options align'.
   void AddAlignmentAttributesForRecord(RecordDecl *RD);
Index: clang/include/clang/Basic/LangOptions.h
===
--- clang/include/clang/Basic/LangOptions.h
+++ clang/include/clang/Basic/LangOptions.h
@@ -178,6 +178,34 @@
 FEA_On
   };
 
+  // Values of the following enumerations correspond to metadata arguments
+  // specified for constrained floating-point intrinsics:
+  // http://llvm.org/docs/LangRef.html#constrained-floating-point-intrinsics.
+
+  /// Possible rounding modes.
+  enum FPRoundingModeKind {
+/// Rounding to nearest, corresponds to "round.tonearest".
+ToNearest,
+/// Rounding toward -Inf, corresponds to "round.downward".
+Downward,
+/// Rounding toward +Inf, corresponds to "round.upward".
+Upward,
+/// Rounding toward zero,