Silence GCC 7 warning by using an enum class.

[fhahn]

Summary

This silences the following GCC7 warning:

lib/Target/Hexagon/HexagonISelDAGToDAGHVX.cpp:142:30: warning:
enumeral and non-enumeral type in conditional expression [-Wextra]
     return F != Colors.end() ? F->second : None;
                ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~

[fhahn]

@RKSimon I saw that you committed a couple of changes fixing some GCC warnings in the Hexagon backend. Thank you very much for that! This patch here fixes the same warning as rL320254, in a different way.

[fhahn]

ping. Please let me know if you think this should be included, otherwise I am happy to drop this.

[davide]

Go ahead if you still see this warning.

[RKSimon]

LGTM

lib/Target/Hexagon/HexagonISelDAGToDAGHVX.cpp

95 ↗

(On Diff #126110)

(style) Worth using a CK_ prefix?

enum class ColorKind { CK_None, CK_Red, CK_Black };

[fhahn]

Thanks for having a look!

lib/Target/Hexagon/HexagonISelDAGToDAGHVX.cpp

95 ↗

(On Diff #126110)

enum classes are strongly scoped, so any access needs to be prefixed with ColorKind::. With that, having a CK_ prefix seems slightly redundant to me. What do you think?

[RKSimon]

lib/Target/Hexagon/HexagonISelDAGToDAGHVX.cpp

95 ↗

(On Diff #126110)

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

[fhahn]

lib/Target/Hexagon/HexagonISelDAGToDAGHVX.cpp

95 ↗

(On Diff #126110)

Hm, it says Unless the enumerators are defined in their own small namespace or .... My understanding is that enum class define the enumerators in their own namespace (although the coding standard could explicitly spell out the enum class case). Also, I had a look at a couple of files with enum classes and all enumerators were without prefix.

[RKSimon]

lib/Target/Hexagon/HexagonISelDAGToDAGHVX.cpp

95 ↗

(On Diff #126110)

Fair enough.

llvm/trunk/lib/Target/Hexagon/HexagonISelDAGToDAGHVX.cpp

 //
 // Reverse delta network is same as delta network, with the steps in
 // the opposite order.
 //
 //
 // Benes network is a forward delta network immediately followed by
 // a reverse delta network.
 
+enum class ColorKind { None, Red, Black };
 
 // Graph coloring utility used to partition nodes into two groups:
 // they will correspond to nodes routed to the upper and lower networks.
 struct Coloring {
-  enum : uint8_t {
-    None = 0,
-    Red,
-    Black
-  };
-
   using Node = int;
-  using MapType = std::map<Node,uint8_t>;
+  using MapType = std::map<Node, ColorKind>;
   static constexpr Node Ignore = Node(-1);
 
   Coloring(ArrayRef<Node> Ord) : Order(Ord) {
     build();
     if (!color())
       Colors.clear();
   }
 
   const MapType &colors() const {
     return Colors;
   }
 
-  uint8_t other(uint8_t Color) {
-    if (Color == None)
-      return Red;
-    return Color == Red ? Black : Red;
+  ColorKind other(ColorKind Color) {
+    if (Color == ColorKind::None)
+      return ColorKind::Red;
+    return Color == ColorKind::Red ? ColorKind::Black : ColorKind::Red;
   }
 
   void dump() const;
 
 private:
   ArrayRef<Node> Order;
   MapType Colors;
   std::set<Node> Needed;
 
   using NodeSet = std::set<Node>;
   std::map<Node,NodeSet> Edges;
 
   Node conj(Node Pos) {
     Node Num = Order.size();
     return (Pos < Num/2) ? Pos + Num/2 : Pos - Num/2;
   }
 
-  uint8_t getColor(Node N) {
+  ColorKind getColor(Node N) {
     auto F = Colors.find(N);
-    return F != Colors.end() ? F->second : (uint8_t)None;
+    return F != Colors.end() ? F->second : ColorKind::None;
   }
 
-  std::pair<bool,uint8_t> getUniqueColor(const NodeSet &Nodes);
+  std::pair<bool, ColorKind> getUniqueColor(const NodeSet &Nodes);
 
   void build();
   bool color();
 };
 } // namespace
 
-std::pair<bool,uint8_t> Coloring::getUniqueColor(const NodeSet &Nodes) {
-  uint8_t Color = None;
+std::pair<bool, ColorKind> Coloring::getUniqueColor(const NodeSet &Nodes) {
+  auto Color = ColorKind::None;
   for (Node N : Nodes) {
-    uint8_t ColorN = getColor(N);
-    if (ColorN == None)
+    ColorKind ColorN = getColor(N);
+    if (ColorN == ColorKind::None)
       continue;
-    if (Color == None)
+    if (Color == ColorKind::None)
       Color = ColorN;
-    else if (Color != None && Color != ColorN)
-      return { false, None };
+    else if (Color != ColorKind::None && Color != ColorN)
+      return { false, ColorKind::None };
   }
   return { true, Color };
 }
 
 void Coloring::build() {
   // Add Order[P] and Order[conj(P)] to Edges.
   for (unsigned P = 0; P != Order.size(); ++P) {
     Node I = Order[P];
for (unsigned I = 0; I < WorkQ.size(); ++I) {
     auto P = getUniqueColor(Ns);
     if (P.first) {
       Colors[N] = other(P.second);
       continue;
     }
 
     // Coloring failed. Split this node.
     Node C = conj(N);
-    uint8_t ColorN = other(None);
-    uint8_t ColorC = other(ColorN);
+    ColorKind ColorN = other(ColorKind::None);
+    ColorKind ColorC = other(ColorN);
     NodeSet &Cs = Edges[C];
     NodeSet CopyNs = Ns;
     for (Node M : CopyNs) {
-      uint8_t ColorM = getColor(M);
+      ColorKind ColorM = getColor(M);
       if (ColorM == ColorC) {
         // Connect M with C, disconnect M from N.
         Cs.insert(M);
         Edges[M].insert(C);
         Ns.erase(M);
         Edges[M].erase(N);
       }
     }
     Colors[N] = ColorN;
     Colors[C] = ColorC;
   }
 
   // Explicitly assign "None" all all uncolored nodes.
   for (unsigned I = 0; I != Order.size(); ++I)
     if (Colors.count(I) == 0)
-      Colors[I] = None;
+      Colors[I] = ColorKind::None;
 
   return true;
 }
 
 LLVM_DUMP_METHOD
 void Coloring::dump() const {
   dbgs() << "{ Order:   {";
   for (unsigned I = 0; I != Order.size(); ++I) {
void Coloring::dump() const {
   for (auto E : Edges) {
     dbgs() << "    " << E.first << " -> {";
     for (auto N : E.second)
       dbgs() << ' ' << N;
     dbgs() << " }\n";
   }
   dbgs() << "  }\n";
 
-  static const char *const Names[] = { "None", "Red", "Black" };
+  auto ColorKindToName = [](ColorKind C) {
+    switch (C) {
+    case ColorKind::None:
+      return "None";
+    case ColorKind::Red:
+      return "Red";
+    case ColorKind::Black:
+      return "Black";
+    }
+    llvm_unreachable("all ColorKinds should be handled by the switch above");
+  };
+
   dbgs() << "  Colors: {\n";
   for (auto C : Colors)
-    dbgs() << "    " << C.first << " -> " << Names[C.second] << "\n";
+    dbgs() << "    " << C.first << " -> " << ColorKindToName(C.second) << "\n";
   dbgs() << "  }\n}\n";
 }
 
 namespace {
 // Base class of for reordering networks. They don't strictly need to be
 // permutations, as outputs with repeated occurrences of an input element
 // are allowed.
 struct PermNetwork {
bool ReverseDeltaNetwork::route(ElemType *P, RowType *T, unsigned Size,
   ElemType Num = Size;
 
   // In this step half-switching occurs, so coloring can be used.
   Coloring G({P,Size});
   const Coloring::MapType &M = G.colors();
   if (M.empty())
     return false;
 
-  uint8_t ColorUp = Coloring::None;
+  ColorKind ColorUp = ColorKind::None;
   for (ElemType J = 0; J != Num; ++J) {
     ElemType I = P[J];
     // I is the position in the input,
     // J is the position in the output.
     if (I == Ignore)
       continue;
-    uint8_t C = M.at(I);
-    if (C == Coloring::None)
+    ColorKind C = M.at(I);
+    if (C == ColorKind::None)
       continue;
     // During "Step", inputs cannot switch halves, so if the "up" color
     // is still unknown, make sure that it is selected in such a way that
     // "I" will stay in the same half.
     bool InpUp = I < Num/2;
-    if (ColorUp == Coloring::None)
+    if (ColorUp == ColorKind::None)
       ColorUp = InpUp ? C : G.other(C);
     if ((C == ColorUp) != InpUp) {
       // If I should go to a different half than where is it now, give up.
       return false;
     }
 
     uint8_t S;
     if (InpUp) {
bool BenesNetwork::route(ElemType *P, RowType *T, unsigned Size,
   ElemType Num = Size;
 
   unsigned Pets = 2*Log-1 - Step;
   bool UseUp = false, UseDown = false;
 
   // Both assignments, i.e. Red->Up and Red->Down are valid, but they will
   // result in different controls. Let's pick the one where the first
   // control will be "Pass".
-  uint8_t ColorUp = Coloring::None;
+  ColorKind ColorUp = ColorKind::None;
   for (ElemType J = 0; J != Num; ++J) {
     ElemType I = P[J];
     if (I == Ignore)
       continue;
-    uint8_t C = M.at(I);
-    if (C == Coloring::None)
+    ColorKind C = M.at(I);
+    if (C == ColorKind::None)
       continue;
-    if (ColorUp == Coloring::None) {
-      ColorUp = (I < Num/2) ? Coloring::Red : Coloring::Black;
+    if (ColorUp == ColorKind::None) {
+      ColorUp = (I < Num / 2) ? ColorKind::Red : ColorKind::Black;
     }
     unsigned CI = (I < Num/2) ? I+Num/2 : I-Num/2;
     if (C == ColorUp) {
       if (I < Num/2)
         T[I][Step] = Pass;
       else
         T[CI][Step] = Switch;
       T[J][Pets] = (J < Num/2) ? Pass : Switch;