[PATCH] D156989: FloatingPointMode: Use -1 for "Dynamic"

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

`Dynamic` cannot have value `-1`, its value must fits 3 bits. Some targets 
encode rounding mode in instructions, RISCV is an example. It allows five 
"real" rounding modes, enlisted now in `RoundingMode` to be specified in a 
special field or FP instruction. This field also can contain a distinct value 
that instructs the processor to take rounding mode from a special register. 
This is a "dynamic" rounding mode in contract to the "static" modes, specified 
by one of "real" values. When `#pragma STC FENV_ACCESS ON` is specified, FP 
operations must use the "dynamic" rounding mode, - they have `Dynamic` in 
FPOptions of their AST nodes.


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

https://reviews.llvm.org/D156989

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

[PATCH] D156989: FloatingPointMode: Use -1 for "Dynamic"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

ping. This enum should just match FLT_ROUNDS and designing ABI around whatever 
this was doing doesn't really make sense


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

https://reviews.llvm.org/D156989

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

[PATCH] D156989: FloatingPointMode: Use -1 for "Dynamic"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D156989#4558486 , @sepavloff wrote:

> Support of rounding mode in C standard is based on IEEE-754 model, where 
> rounding mode is a global state and affects all FP operations. The case of 
> two rounding modes does not fit this model. So in C/C++ you anyway need to 
> invent special tools for setting this or that rounding mode or reading them. 
> If static rounding mode is not needed, IEEE-754 rounding mode could be 
> represented by `Dynamic` value.

Correct, I'm not planning on creating special "standard looking" tools. I just 
want the value in the target defined range rather than a black box.

> In IR there are more possibilities to represent many rounding modes. Each 
> constrained intrinsic call contains rounding mode and that mode may be 
> different for different FP types. Actually this model can support the general 
> case. For example, rounding mode for one type can be static but for the other 
> type it can be dynamic. There must be intrinsic functions that set/get 
> rounding mode for different types.

The raw target intrinsic to read the mode register works. Once you're in the 
target range you know you have to do something with target operations

> It looks like adding special bultin functions to get/set rounding mode for 
> different types is enough to support rounding in AMDGPU. In any case IEEE-754 
> rounding mode should be honored, which means that `fegetround` and 
> `FLT_ROUNDS` probably should return negative value, and `fesetround` probably 
> should set all rounding modes. The difference between `Dynamic` and -1 does 
> not matter because `Dynamic` can never be an argument of rounding mode type 
> and `Invalid` (-1) is an error indicator and must not be treated as rounding 
> mode.

My interpretation is fesetround of standard values would set all modes to be 
the same. Once you're outside of the range  it would work correctly to consume 
the same target defined values. "Could not determine" is the same as dynamic, 
so this should just use the standard -1 value instead of 7 just to fit in a 
clang bitfield


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

https://reviews.llvm.org/D156989

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

[PATCH] D156989: FloatingPointMode: Use -1 for "Dynamic"

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D156989#4558134 , @arsenm wrote:

> In D156989#4558133 , @sepavloff 
> wrote:
>
>> Rounding mode is presented in FPOptions with 3 bits, so there is only 8 
>> values available for particular modes. 5 of them, which are specified in 
>> IEEE-754, are listed in `RoundingMode`. `Dynamic` (which is -1 in 3-bit 
>> numbers) is not a real rounding mode,
>
> But it is a spec'd value as -1 for FLT_ROUNDS

It is no more than failure indicator for this function or macro. `fegetround` 
may use any negative value for this purpose. -1 does not represent any rounding 
mode, just like EOF does not represent any character in a stream.

>> Probably `Dynamic` is what you need. It prevents from constant folding and 
>> other transformations that rely on particular rounding mode and does not 
>> restrict actual rounding modes used in runtime. What  prevents from using 
>> this mode for your case?
>
> I can do better by reporting something meaningful, two different modes is not 
> unknown. The enum here should just be exactly equal to the FLT_ROUNDS values 
> and not pick a random other number, I just need the wrong value for Dynamic 
> to get out of the way to avoid creating additional wrappers

Support of rounding mode in C standard is based on IEEE-754 model, where 
rounding mode is a global state and affects all FP operations. The case of two 
rounding modes does not fit this model. So in C/C++ you anyway need to invent 
special tools for setting this or that rounding mode or reading them. If static 
rounding mode is not needed, IEEE-754 rounding mode could be represented by 
`Dynamic` value.

In IR there are more possibilities to represent many rounding modes. Each 
constrained intrinsic call contains rounding mode and that mode may be 
different for different FP types. Actually this model can support the general 
case. For example, rounding mode for one type can be static but for the other 
type it can be dynamic. There must be intrinsic functions that set/get rounding 
mode for different types.

It looks like adding special bultin functions to get/set rounding mode for 
different types is enough to support rounding in AMDGPU. In any case IEEE-754 
rounding mode should be honored, which means that `fegetround` and `FLT_ROUNDS` 
probably should return negative value, and `fesetround` probably should set all 
rounding modes. The difference between `Dynamic` and -1 does not matter because 
`Dynamic` can never be an argument of rounding mode type and `Invalid` (-1) is 
an error indicator and must not be treated as rounding mode.


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

https://reviews.llvm.org/D156989

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

[PATCH] D156989: FloatingPointMode: Use -1 for "Dynamic"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D156989#4558133 , @sepavloff wrote:

> Rounding mode is presented in FPOptions with 3 bits, so there is only 8 
> values available for particular modes. 5 of them, which are specified in 
> IEEE-754, are listed in `RoundingMode`. `Dynamic` (which is -1 in 3-bit 
> numbers) is not a real rounding mode,

But it is a spec'd value as -1 for FLT_ROUNDS

> `RoundingMode::Invalid` is not a mode at all, it is used to represent 
> unspecified value at compile-time and can be eliminated by using things like 
> `std::optional`. In 3 bits it would have the same value as `Dynamic`, but it 
> is not a problem, because `Invalid` never appears in AST and IR.

Right it's just filler here

> Probably `Dynamic` is what you need. It prevents from constant folding and 
> other transformations that rely on particular rounding mode and does not 
> restrict actual rounding modes used in runtime. What  prevents from using 
> this mode for your case?

I can do better by reporting something meaningful, two different modes is not 
unknown. The enum here should just be exactly equal to the FLT_ROUNDS values 
and not pick a random other number, I just need the wrong value for Dynamic to 
get out of the way to avoid creating additional wrappers


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

https://reviews.llvm.org/D156989

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

[PATCH] D156989: FloatingPointMode: Use -1 for "Dynamic"

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

Rounding mode is presented in FPOptions with 3 bits, so there is only 8 values 
available for particular modes. 5 of them, which are specified in IEEE-754, are 
listed in `RoundingMode`. `Dynamic` (which is -1 in 3-bit numbers) is not a 
real rounding mode, it represents rounding mode unknown at compiler time. 
`RoundingMode::Invalid` is not a mode at all, it is used to represent 
unspecified value at compile-time and can be eliminated by using things like 
`std::optional`. In 3 bits it would have the same value as `Dynamic`, but it is 
not a problem, because `Invalid` never appears in AST and IR.

Probably `Dynamic` is what you need. It prevents from constant folding and 
other transformations that rely on particular rounding mode and does not 
restrict actual rounding modes used in runtime. What  prevents from using this 
mode for your case?


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

https://reviews.llvm.org/D156989

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

[PATCH] D156989: FloatingPointMode: Use -1 for "Dynamic"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm updated this revision to Diff 546815.

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

https://reviews.llvm.org/D156989

Files:
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/LangOptions.cpp
  llvm/include/llvm/ADT/FloatingPointMode.h

Index: llvm/include/llvm/ADT/FloatingPointMode.h
===
--- llvm/include/llvm/ADT/FloatingPointMode.h
+++ llvm/include/llvm/ADT/FloatingPointMode.h
@@ -35,18 +35,28 @@
 /// rounding mode value, so it does not need to fit the bit fields.
 ///
 enum class RoundingMode : int8_t {
+  // Special values.
+  Invalid = -2, ///< Denotes invalid value.
+  Dynamic = -1, ///< Denotes mode unknown at compile time.
+
   // Rounding mode defined in IEEE-754.
   TowardZero= 0,///< roundTowardZero.
   NearestTiesToEven = 1,///< roundTiesToEven.
   TowardPositive= 2,///< roundTowardPositive.
   TowardNegative= 3,///< roundTowardNegative.
-  NearestTiesToAway = 4,///< roundTiesToAway.
-
-  // Special values.
-  Dynamic = 7,///< Denotes mode unknown at compile time.
-  Invalid = -1///< Denotes invalid value.
+  NearestTiesToAway = 4 ///< roundTiesToAway.
 };
 
+/// Encode a RoundingMode value into a form suitable for a bitfield.
+constexpr int8_t encodeRoundingMode(RoundingMode RM) {
+  return static_cast(RM) + 1;
+}
+
+/// Decode a RoundingMode value from a form suitable for a bitfield.
+constexpr RoundingMode decodeRoundingMode(int8_t Val) {
+  return static_cast(Val - 1);
+}
+
 /// Returns text representation of the given rounding mode.
 inline StringRef spell(RoundingMode RM) {
   switch (RM) {
Index: clang/lib/Basic/LangOptions.cpp
===
--- clang/lib/Basic/LangOptions.cpp
+++ clang/lib/Basic/LangOptions.cpp
@@ -215,7 +215,7 @@
 
 FPOptionsOverride FPOptions::getChangesSlow(const FPOptions ) const {
   FPOptions::storage_type OverrideMask = 0;
-#define OPTION(NAME, TYPE, WIDTH, PREVIOUS)\
+#define OPTION(NAME, TYPE, ENCODE, DECODE, WIDTH, PREVIOUS)\
   if (get##NAME() != Base.get##NAME()) \
 OverrideMask |= NAME##Mask;
 #include "clang/Basic/FPOptions.def"
@@ -223,14 +223,14 @@
 }
 
 LLVM_DUMP_METHOD void FPOptions::dump() {
-#define OPTION(NAME, TYPE, WIDTH, PREVIOUS)\
+#define OPTION(NAME, TYPE, ENCODE, DECODE, WIDTH, PREVIOUS)\
   llvm::errs() << "\n " #NAME " " << get##NAME();
 #include "clang/Basic/FPOptions.def"
   llvm::errs() << "\n";
 }
 
 LLVM_DUMP_METHOD void FPOptionsOverride::dump() {
-#define OPTION(NAME, TYPE, WIDTH, PREVIOUS)\
+#define OPTION(NAME, TYPE, ENCODE, DECODE, WIDTH, PREVIOUS)\
   if (has##NAME##Override())   \
 llvm::errs() << "\n " #NAME " Override is " << get##NAME##Override();
 #include "clang/Basic/FPOptions.def"
Index: clang/lib/AST/TextNodeDumper.cpp
===
--- clang/lib/AST/TextNodeDumper.cpp
+++ clang/lib/AST/TextNodeDumper.cpp
@@ -756,7 +756,7 @@
 }
 
 void TextNodeDumper::printFPOptions(FPOptionsOverride FPO) {
-#define OPTION(NAME, TYPE, WIDTH, PREVIOUS)\
+#define OPTION(NAME, TYPE, ENCODE, DECODE, WIDTH, PREVIOUS)\
   if (FPO.has##NAME##Override())   \
 OS << " " #NAME "=" << FPO.get##NAME##Override();
 #include "clang/Basic/FPOptions.def"
Index: clang/lib/AST/JSONNodeDumper.cpp
===
--- clang/lib/AST/JSONNodeDumper.cpp
+++ clang/lib/AST/JSONNodeDumper.cpp
@@ -1740,7 +1740,7 @@
 
 llvm::json::Object JSONNodeDumper::createFPOptions(FPOptionsOverride FPO) {
   llvm::json::Object Ret;
-#define OPTION(NAME, TYPE, WIDTH, PREVIOUS)\
+#define OPTION(NAME, TYPE, ENCODE, DECODE, WIDTH, PREVIOUS)\
   if (FPO.has##NAME##Override())   \
 Ret.try_emplace(#NAME, static_cast(FPO.get##NAME##Override()));
 #include "clang/Basic/FPOptions.def"
Index: clang/include/clang/Basic/LangOptions.h
===
--- clang/include/clang/Basic/LangOptions.h
+++ clang/include/clang/Basic/LangOptions.h
@@ -670,7 +670,7 @@
   // Define a fake option named "First" so that we have a PREVIOUS even for the
   // real first option.
   static constexpr storage_type FirstShift = 0, FirstWidth = 0;
-#define OPTION(NAME, TYPE, WIDTH, PREVIOUS)\
+#define OPTION(NAME, ENCODE, DECODE, TYPE, WIDTH, 

[PATCH] D156989: FloatingPointMode: Use -1 for "Dynamic"

[Jay Foad via Phabricator via cfe-commits]

foad added inline comments.



Comment at: llvm/include/llvm/ADT/FloatingPointMode.h:39
+  // Special values.
+  Invalid = -2,
+

Lost the `///<` comment here.



Comment at: llvm/include/llvm/ADT/FloatingPointMode.h:41
+
+  ///< Denotes mode unknown at compile time.
+  Dynamic = -1,

Does `///<` work //before// the field name?


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

https://reviews.llvm.org/D156989

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

[PATCH] D156989: FloatingPointMode: Use -1 for "Dynamic"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm created this revision.
arsenm added reviewers: sepavloff, rjmccall, kpn, cameron.mcinally, uweigand, 
scanon, jcranmer-intel, foad.
Herald added subscribers: StephenFan, tpr.
Herald added a project: All.
arsenm requested review of this revision.
Herald added a subscriber: wdng.
Herald added a project: LLVM.

FLT_ROUNDS says -1 is used for "the default rounding direction is not
known". The previous 7 was taking away options in the
"implementation-defined behavior" range if you just wanted to extend
the enum.

  

AMDGPU has 2 separately controllable rounding modes that change
different fp types. I want to stick to the standard values in the case
the modes are the same, and use the extended range for cases where the
two are different. Dodging this gap in the enum value required
defining the AMDGPU target specific values in a weird way with strange
conversion code to handle it (see https://reviews.llvm.org/D153257).


https://reviews.llvm.org/D156989

Files:
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/LangOptions.cpp
  llvm/include/llvm/ADT/FloatingPointMode.h

Index: llvm/include/llvm/ADT/FloatingPointMode.h
===
--- llvm/include/llvm/ADT/FloatingPointMode.h
+++ llvm/include/llvm/ADT/FloatingPointMode.h
@@ -35,18 +35,30 @@
 /// rounding mode value, so it does not need to fit the bit fields.
 ///
 enum class RoundingMode : int8_t {
+  // Special values.
+  Invalid = -2,
+
+  ///< Denotes mode unknown at compile time.
+  Dynamic = -1,
+
   // Rounding mode defined in IEEE-754.
   TowardZero= 0,///< roundTowardZero.
   NearestTiesToEven = 1,///< roundTiesToEven.
   TowardPositive= 2,///< roundTowardPositive.
   TowardNegative= 3,///< roundTowardNegative.
-  NearestTiesToAway = 4,///< roundTiesToAway.
-
-  // Special values.
-  Dynamic = 7,///< Denotes mode unknown at compile time.
-  Invalid = -1///< Denotes invalid value.
+  NearestTiesToAway = 4 ///< roundTiesToAway.
 };
 
+/// Encode a RoundingMode value into a form suitable for a bitfield.
+constexpr int8_t encodeRoundingMode(RoundingMode RM) {
+  return static_cast(RM) + 1;
+}
+
+/// Decode a RoundingMode value from a form suitable for a bitfield.
+constexpr RoundingMode decodeRoundingMode(int8_t Val) {
+  return static_cast(Val - 1);
+}
+
 /// Returns text representation of the given rounding mode.
 inline StringRef spell(RoundingMode RM) {
   switch (RM) {
Index: clang/lib/Basic/LangOptions.cpp
===
--- clang/lib/Basic/LangOptions.cpp
+++ clang/lib/Basic/LangOptions.cpp
@@ -215,7 +215,7 @@
 
 FPOptionsOverride FPOptions::getChangesSlow(const FPOptions ) const {
   FPOptions::storage_type OverrideMask = 0;
-#define OPTION(NAME, TYPE, WIDTH, PREVIOUS)\
+#define OPTION(NAME, TYPE, ENCODE, DECODE, WIDTH, PREVIOUS)\
   if (get##NAME() != Base.get##NAME()) \
 OverrideMask |= NAME##Mask;
 #include "clang/Basic/FPOptions.def"
@@ -223,14 +223,14 @@
 }
 
 LLVM_DUMP_METHOD void FPOptions::dump() {
-#define OPTION(NAME, TYPE, WIDTH, PREVIOUS)\
+#define OPTION(NAME, TYPE, ENCODE, DECODE, WIDTH, PREVIOUS)\
   llvm::errs() << "\n " #NAME " " << get##NAME();
 #include "clang/Basic/FPOptions.def"
   llvm::errs() << "\n";
 }
 
 LLVM_DUMP_METHOD void FPOptionsOverride::dump() {
-#define OPTION(NAME, TYPE, WIDTH, PREVIOUS)\
+#define OPTION(NAME, TYPE, ENCODE, DECODE, WIDTH, PREVIOUS)\
   if (has##NAME##Override())   \
 llvm::errs() << "\n " #NAME " Override is " << get##NAME##Override();
 #include "clang/Basic/FPOptions.def"
Index: clang/lib/AST/TextNodeDumper.cpp
===
--- clang/lib/AST/TextNodeDumper.cpp
+++ clang/lib/AST/TextNodeDumper.cpp
@@ -756,7 +756,7 @@
 }
 
 void TextNodeDumper::printFPOptions(FPOptionsOverride FPO) {
-#define OPTION(NAME, TYPE, WIDTH, PREVIOUS)\
+#define OPTION(NAME, TYPE, ENCODE, DECODE, WIDTH, PREVIOUS)\
   if (FPO.has##NAME##Override())   \
 OS << " " #NAME "=" << FPO.get##NAME##Override();
 #include "clang/Basic/FPOptions.def"
Index: clang/lib/AST/JSONNodeDumper.cpp
===
--- clang/lib/AST/JSONNodeDumper.cpp
+++ clang/lib/AST/JSONNodeDumper.cpp
@@ -1740,7 +1740,7 @@
 
 llvm::json::Object JSONNodeDumper::createFPOptions(FPOptionsOverride FPO) {
   llvm::json::Object Ret;
-#define OPTION(NAME, TYPE, WIDTH, PREVIOUS)\