Add a trait to Sequence to iterate enums

[gchatelet]

Summary

By default llvm::Sequence will happily iterate over enums, this patch forces the user to declare the enum iterable before being able to do so.
This emerged from a discussion with kuhar@ about reusing llvm's Sequence in lieu of https://github.com/GPUOpen-Drivers/llpc/blob/dev/lgc/interface/lgc/EnumIterator.h.

This patch is to discuss the design points (guard against enum iteration, provide tighter bounds, etc...).

[gchatelet]

Several observations:

1. It is entirely possible to hijack the system by providing the macro where you actually need to iterate the enum (see llvm/tools/llvm-exegesis/lib/X86/Target.cpp). That is even if the enum is not iterable, adding the macro in a different file will work fine.
2. One of the desirable property was to be able to mark the enum as iterable next to its definition. I wasn't able to do so for any of the enums I tagged. The macro usually sits at the end of the file (far from its definition) because the enums are nested in classes and you can't specialize the template here.
3. This design actually forces to #include "llvm/ADT/Sequence.h" in any file that declares an iterable enum even though there's no direct need to iterate the enum.
4. In the tests, I had to change the structure of the code completely to be able to make it work. It is likely that this is going to be difficult to use in a different codebase where the root namespace is not llvm.

The existence of 1. suggests that clients will declare the macro where they need it, bypassing the security mechanism.
As-is, I fear it doesn't bring a lot of value, @kuhar WDYT? Any suggestions on how to make it nicer to use / safer?

[kuhar]

Thanks for looking into this, @gchatelet

As-is, I fear it doesn't bring a lot of value, @kuhar WDYT? Any suggestions on how to make it nicer to use / safer?

Yes, the trait-based implementation is not the easiest to use like this, especially with projects using different namespaces and nested enums.

What if instead we put traits together with enum values, and have a function (customization point) to augment an enum value with its traits? Then we can have a way to opt into enum iteration even for enums that aren't declared as iterable. I made a quick proof of concept and uploaded it here: https://reviews.llvm.org/D107378.

[gchatelet]

In D107350#2923018, @kuhar wrote:

What if instead we put traits together with enum values, and have a function (customization point) to augment an enum value with its traits? Then we can have a way to opt into enum iteration even for enums that aren't declared as iterable. I made a quick proof of concept and uploaded it here: https://reviews.llvm.org/D107378.

Thx for the patch.
I gave it a try and it seems it does not work in the context of the tests because of "warning as errors".

/redacted/git/llvm-project/llvm/unittests/ADT/SequenceTest.cpp:210:1: error: unused function 'add_enum_traits' [-Werror,-Wunused-function]
DECLARE_ITERABLE_ENUM(UntypedEnum)
^
/redacted/git/llvm-project/llvm/include/llvm/ADT/Sequence.h:52:15: note: expanded from macro 'DECLARE_ITERABLE_ENUM'
  inline auto add_enum_traits(ENUM value) {                                    \
              ^

Unfortunately marking the function as static will not work when the function is declared in a class (static has a different meaning there)

/redacted/git/llvm-project/llvm/include/llvm/Support/MachineValueType.h:1406:12: error: 'static' is invalid in friend declarations
    friend DECLARE_ITERABLE_ENUM(MVT::SimpleValueType)
           ^
/redacted/git/llvm-project/llvm/include/llvm/ADT/Sequence.h:52:3: note: expanded from macro 'DECLARE_ITERABLE_ENUM'
  static inline auto add_enum_traits(ENUM value) {                                    \
  ^

The idea is interesting though, maybe there's a way to make it work? We could use different macros depending on context but ideally a single one would be best.

AFAIU the proposal is about making sure that we don't iterate on an enum by mistake but in your patch it seems that calling make_iterable_enum bypasses this security (i.e. X86::CondCode has never been declared iterable ). Was that intended?

[kuhar]

In D107350#2937011, @gchatelet wrote:

Thx for the patch.
I gave it a try and it seems it does not work in the context of the tests because of "warning as errors".

/redacted/git/llvm-project/llvm/unittests/ADT/SequenceTest.cpp:210:1: error: unused function 'add_enum_traits' [-Werror,-Wunused-function]
DECLARE_ITERABLE_ENUM(UntypedEnum)
^
/redacted/git/llvm-project/llvm/include/llvm/ADT/Sequence.h:52:15: note: expanded from macro 'DECLARE_ITERABLE_ENUM'
  inline auto add_enum_traits(ENUM value) {                                    \
              ^

Unfortunately marking the function as static will not work when the function is declared in a class (static has a different meaning there)

/redacted/git/llvm-project/llvm/include/llvm/Support/MachineValueType.h:1406:12: error: 'static' is invalid in friend declarations
    friend DECLARE_ITERABLE_ENUM(MVT::SimpleValueType)
           ^
/redacted/git/llvm-project/llvm/include/llvm/ADT/Sequence.h:52:3: note: expanded from macro 'DECLARE_ITERABLE_ENUM'
  static inline auto add_enum_traits(ENUM value) {                                    \
  ^

The idea is interesting though, maybe there's a way to make it work? We could use different macros depending on context but ideally a single one would be best.

I did not build with warnings as errors and didn't realize this was a problem. I think we should be able to add an LLVM_ATTRIBUTE_UNUSED to the functions: https://clang.llvm.org/docs/AttributeReference.html#id363.

AFAIU the proposal is about making sure that we don't iterate on an enum by mistake but in your patch it seems that calling make_iterable_enum bypasses this security (i.e. X86::CondCode has never been declared iterable ). Was that intended?

Yes, it was intentional. This is the 'escape hatch' that I mentioned. This is so that it's still possible to iterate over enums when the declaration is missing, for example, when we cannot modify the code where the enum comes from (think an external library), or know that only a part of enum values is iterable and don't want to declare the whole thing as safe to iterate over.

[gchatelet]

In D107350#2937039, @kuhar wrote:

AFAIU the proposal is about making sure that we don't iterate on an enum by mistake but in your patch it seems that calling make_iterable_enum bypasses this security (i.e. X86::CondCode has never been declared iterable ). Was that intended?

Yes, it was intentional. This is the 'escape hatch' that I mentioned. This is so that it's still possible to iterate over enums when the declaration is missing, for example, when we cannot modify the code where the enum comes from (think an external library), or know that only a part of enum values is iterable and don't want to declare the whole thing as safe to iterate over.

Ah I see, then maybe the name should raise a visual warning, How about forge_iterable_enum?

SG for the unused attribute.

I'll update this patch based on your implementation and we can iterate from here.

[kuhar]

In D107350#2937092, @gchatelet wrote:

Ah I see, then maybe the name should raise a visual warning, How about forge_iterable_enum?

SG for the unused attribute.

I'll update this patch based on your implementation and we can iterate from here.

SGTM.

I talked with @gchatelet offline and since he will be on vacation soon, I'll take over the implementation and continue in the other patch (https://reviews.llvm.org/D107378) from here.
One concern brought up offline was that we should avoid the duplication the integer vs enum logic between seq and seq_inclusive (the two notions are no more orthogonal). We should try to push the enum logic to iota_range instead.

[gchatelet]

Abandoning this patch, conversation moved to D107378

llvm/include/llvm/ADT/Sequence.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ADT_SEQUENCE_H
 #define LLVM_ADT_SEQUENCE_H
 
 #include <cassert>     // assert
 #include <iterator>    // std::random_access_iterator_tag
 #include <limits>      // std::numeric_limits
-#include <type_traits> // std::underlying_type, std::is_enum
+#include <type_traits> // std::underlying_type, std::is_enum, std::false_type
 
 #include "llvm/Support/MathExtras.h" // AddOverflow / SubOverflow
 
 namespace llvm {
 
+template <typename Enum> struct is_iterable_enum : public std::false_type {};
+
+#define DECLARE_ITERABLE_ENUM(ENUM)                                            \
+  template <> struct is_iterable_enum<ENUM> : public std::true_type {}
+
 namespace detail {
 
 // Returns whether a value of type U can be represented with type T.
 template <typename T, typename U> bool canTypeFitValue(const U Value) {
   const intmax_t BotT = intmax_t(std::numeric_limits<T>::min());
   const intmax_t BotU = intmax_t(std::numeric_limits<U>::min());
   const uintmax_t TopT = uintmax_t(std::numeric_limits<T>::max());
   const uintmax_t TopU = uintmax_t(std::numeric_limits<U>::max());
static CheckedInt from(Integral FromValue) {
     Result.Value = static_cast<intmax_t>(FromValue);
     return Result;
   }
 
   // Enum constructor, asserts if Value cannot be represented as intmax_t.
   template <typename Enum,
             typename std::enable_if_t<std::is_enum<Enum>::value, bool> = 0>
   static CheckedInt from(Enum FromValue) {
+    static_assert(llvm::is_iterable_enum<Enum>::value,
+                  "Enum type is not iterable, use DECLARE_ITERABLE_ENUM");
     using type = typename std::underlying_type<Enum>::type;
     return from<type>(static_cast<type>(FromValue));
   }
 
   // Equality
   bool operator==(const CheckedInt &O) const { return Value == O.Value; }
   bool operator!=(const CheckedInt &O) const { return Value != O.Value; }
 

llvm/include/llvm/IR/InstrTypes.h

 
 #ifndef LLVM_IR_INSTRTYPES_H
 #define LLVM_IR_INSTRTYPES_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/Sequence.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/CallingConv.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DerivedTypes.h"

 };
 
 template <>
 struct OperandTraits<FuncletPadInst>
     : public VariadicOperandTraits<FuncletPadInst, /*MINARITY=*/1> {};
 
 DEFINE_TRANSPARENT_OPERAND_ACCESSORS(FuncletPadInst, Value)
 
+DECLARE_ITERABLE_ENUM(CmpInst::Predicate);
+
 } // end namespace llvm
 
 #endif // LLVM_IR_INSTRTYPES_H

llvm/include/llvm/Support/MachineValueType.h

public:
 
     static auto fp_scalable_vector_valuetypes() {
       return seq_inclusive(MVT::FIRST_FP_SCALABLE_VECTOR_VALUETYPE,
                            MVT::LAST_FP_SCALABLE_VECTOR_VALUETYPE);
     }
     /// @}
   };
 
+  DECLARE_ITERABLE_ENUM(MVT::SimpleValueType);
+
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_MACHINEVALUETYPE_H

llvm/tools/llvm-exegesis/lib/X86/Target.cpp

 #include <string>
 #include <vector>
 #if defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))
 #include <immintrin.h>
 #include <intrin.h>
 #endif
 
 namespace llvm {
+
+DECLARE_ITERABLE_ENUM(X86::CondCode);
+
 namespace exegesis {
 
 static cl::OptionCategory
     BenchmarkOptions("llvm-exegesis benchmark x86-options");
 
 // If a positive value is specified, we are going to use the LBR in
 // latency-mode.
 //

 #include "X86GenExegesis.inc"
 
 namespace {
 
 class X86SavedState : public ExegesisTarget::SavedState {
 public:
   X86SavedState() {
 #ifdef __x86_64__
-# if defined(_MSC_VER)
+#if defined(_MSC_VER)
     _fxsave64(FPState);
     Eflags = __readeflags();
-# elif defined(__GNUC__)
+#elif defined(__GNUC__)
     __builtin_ia32_fxsave64(FPState);
     Eflags = __builtin_ia32_readeflags_u64();
-# endif
+#endif
 #else
     llvm_unreachable("X86 exegesis running on non-X86 target");
 #endif
   }
 
   ~X86SavedState() {
     // Restoring the X87 state does not flush pending exceptions, make sure
     // these exceptions are flushed now.
 #ifdef __x86_64__
-# if defined(_MSC_VER)
+#if defined(_MSC_VER)
     _clearfp();
     _fxrstor64(FPState);
     __writeeflags(Eflags);
-# elif defined(__GNUC__)
+#elif defined(__GNUC__)
     asm volatile("fwait");
     __builtin_ia32_fxrstor64(FPState);
     __builtin_ia32_writeeflags_u64(Eflags);
-# endif
+#endif
 #else
     llvm_unreachable("X86 exegesis running on non-X86 target");
 #endif
   }
 
 private:
 #ifdef __x86_64__
   alignas(16) char FPState[512];

llvm/unittests/ADT/SequenceTest.cpp

 
 #include <algorithm>
 #include <numeric>
 
 using namespace llvm;
 
 using testing::ElementsAre;
 
-namespace {
+namespace llvm {
 
 using detail::canTypeFitValue;
 using detail::CheckedInt;
 
 using IntegralTypes = testing::Types<uint8_t,   // 0
                                      uint16_t,  // 1
                                      uint32_t,  // 2
                                      uint64_t,  // 3
TYPED_TEST(StrongIntTest, Operations) {
   EXPECT_NE(Actual, Actual + 1);
 
   // Difference
   EXPECT_EQ(Actual - Actual, 0);
   EXPECT_EQ((Actual + 1) - Actual, 1);
   EXPECT_EQ(Actual - (Actual + 2), -2);
 }
 
-TEST(StrongIntTest, Enums) {
-  enum UntypedEnum { A = 3 };
-  EXPECT_EQ(CheckedInt::from(A).to<UntypedEnum>(), A);
-
-  enum TypedEnum : uint32_t { B = 3 };
-  EXPECT_EQ(CheckedInt::from(B).to<TypedEnum>(), B);
-
-  enum class ScopedEnum : uint16_t { C = 3 };
-  EXPECT_EQ(CheckedInt::from(ScopedEnum::C).to<ScopedEnum>(), ScopedEnum::C);
-}
-
 #if defined(GTEST_HAS_DEATH_TEST) && !defined(NDEBUG)
 TEST(StrongIntDeathTest, OutOfBounds) {
   // Values above 'INTMAX_MAX' are not representable.
   EXPECT_DEATH(CheckedInt::from<uintmax_t>(INTMAX_MAX + 1ULL), "Out of bounds");
   EXPECT_DEATH(CheckedInt::from<uintmax_t>(UINTMAX_MAX), "Out of bounds");
   // Casting to narrower type asserts when out of bounds.
   EXPECT_DEATH(CheckedInt::from(-1).to<uint8_t>(), "Out of bounds");
   EXPECT_DEATH(CheckedInt::from(256).to<uint8_t>(), "Out of bounds");
TEST(SequenceTest, Dereference) {
   const auto Forward = seq(0, 10).begin();
   EXPECT_EQ(Forward[0], 0);
   EXPECT_EQ(Forward[2], 2);
   const auto Backward = seq(0, 10).rbegin();
   EXPECT_EQ(Backward[0], 9);
   EXPECT_EQ(Backward[2], 7);
 }
 
-} // anonymous namespace
+enum UntypedEnum { A = 3 };
+enum TypedEnum : uint32_t { B = 3 };
+enum class ScopedEnum : uint16_t { C = 3 };
+
+DECLARE_ITERABLE_ENUM(UntypedEnum);
+DECLARE_ITERABLE_ENUM(TypedEnum);
+DECLARE_ITERABLE_ENUM(ScopedEnum);
+
+TEST(StrongIntTest, Enums) {
+  EXPECT_EQ(CheckedInt::from(A).to<UntypedEnum>(), A);
+  EXPECT_EQ(CheckedInt::from(B).to<TypedEnum>(), B);
+  EXPECT_EQ(CheckedInt::from(ScopedEnum::C).to<ScopedEnum>(), ScopedEnum::C);
+}
+} // namespace llvm