Page MenuHomePhabricator

Add a trait to Sequence to iterate enums
AbandonedPublic

Authored by gchatelet on Aug 3 2021, 7:07 AM.

Details

Reviewers
kuhar
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...).

Diff Detail

Event Timeline

gchatelet created this revision.Aug 3 2021, 7:07 AM
gchatelet requested review of this revision.Aug 3 2021, 7:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2021, 7:07 AM

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 added a comment.Aug 3 2021, 11:03 AM

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.

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 added a comment.Aug 10 2021, 8:04 AM

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.

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 added a comment.Aug 10 2021, 8:55 AM

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 abandoned this revision.Oct 28 2021, 8:07 AM

Abandoning this patch, conversation moved to D107378