Replace function template (plus explicit specializations) by non-template overloads.

[tkoeppe]

Summary

For the design in question, overloads seem to be a much simpler and less subtle solution.

This removes ODR issues, and errors of the kind where code that uses the specialization in question will accidentally and erroneously specialize the primary template. This only "works" by accident; the program is ill-formed NDR.

(Found with -Wundefined-func-template.)

[chandlerc]

lib/Target/Hexagon/RDFGraph.h

941–942 ↗

(On Diff #189400)

Should probably move the comment below up here and explain more what this is doing. IE: that this template is explicitly specialized for specific types only, etc.

944–945 ↗

(On Diff #189400)

FWIW, I don't think you need to add the forward reference to RDFLiveness.h here.

[tkoeppe]

Moved comment up to the declaration of the primary template and removed mention of other specializations in different headers.

[tkoeppe]

lib/Target/Hexagon/RDFGraph.h

944–945 ↗

(On Diff #189400)

I omitted this when I moved the comment up.

[jyknight]

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

[tkoeppe]

In D58998#1419205, @jyknight wrote:

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

They could, but...

a) I wanted to make a minimal change and respect the original design,
b) Richard Smith told me offline that he had also considered it but doesn't much care either way, and
c) I was suspecting some attempt to speed up compilation by keeping the overload set minimal, which I didn't want to second-guess lightly.

[dblaikie]

In D58998#1419205, @jyknight wrote:

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

I'll second this - and I'm happy to take ownership (if it's needed) of the decision. This code came in with this specialization approach, and I'm going to guess it wasn't for any particularly nuanced reason & is fine to change to overloads as would be more common here.

[tkoeppe]

In D58998#1421799, @dblaikie wrote:

In D58998#1419205, @jyknight wrote:

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

I'll second this - and I'm happy to take ownership (if it's needed) of the decision. This code came in with this specialization approach, and I'm going to guess it wasn't for any particularly nuanced reason & is fine to change to overloads as would be more common here.

The only reason I can think of is that we wanted to keep the overload set small (or trivial), but I don't know if that's significant. (Richard said he doesn't care either way.)

What shall we do now? Would you like to take this change and then refactor, or immediately change this to overloads?

[dblaikie]

In D58998#1422074, @tkoeppe wrote:

In D58998#1421799, @dblaikie wrote:

In D58998#1419205, @jyknight wrote:

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

I'll second this - and I'm happy to take ownership (if it's needed) of the decision. This code came in with this specialization approach, and I'm going to guess it wasn't for any particularly nuanced reason & is fine to change to overloads as would be more common here.

The only reason I can think of is that we wanted to keep the overload set small (or trivial), but I don't know if that's significant. (Richard said he doesn't care either way.)

What shall we do now? Would you like to take this change and then refactor, or immediately change this to overloads?

I'd just immediately switch to overloads as a means to fix the original issue rather than do a two-step.

[tkoeppe]

Ah, another reason to prefer templates might be that templates deduce, and overloads convert. With the template, you can be sure that there aren't any accidental conversions (which would hit the deleted primary template).

Do you think that matters? If so, we can preserve that behaviour by retaining the deleted primary template (but NOT defining any explicit specializations).

[tkoeppe]

Changed function template plus specializations to function overloads.

[dblaikie]

Looks good to me, thanks

Could you make sure you've run this change through clang-format before submitting? (Or if you do not have commit rights, I can format/commit it for you)

[tkoeppe]

In D58998#1423838, @dblaikie wrote:

Could you make sure you've run this change through clang-format before submitting? (Or if you do not have commit rights, I can format/commit it for you)

I'm pretty sure I don't have those; could you please? Thanks a lot!

(I take it that tests have already run and passed for this?)

llvm/trunk/lib/Target/Hexagon/RDFGraph.h

NodeList CodeNode::members_if(Predicate P, const DataFlowGraph &G) const {
     while (M.Addr != this) {
       if (P(M))
         MM.push_back(M);
       M = G.addr<NodeBase*>(M.Addr->getNext());
     }
     return MM;
   }
 
-  template <typename T> struct Print;
-  template <typename T>
-  raw_ostream &operator<< (raw_ostream &OS, const Print<T> &P);
-
   template <typename T>
   struct Print {
     Print(const T &x, const DataFlowGraph &g) : Obj(x), G(g) {}
 
     const T &Obj;
     const DataFlowGraph &G;
   };
 
   template <typename T>
   struct PrintNode : Print<NodeAddr<T>> {
     PrintNode(const NodeAddr<T> &x, const DataFlowGraph &g)
       : Print<NodeAddr<T>>(x, g) {}
   };
 
+  raw_ostream &operator<<(raw_ostream &OS, const Print<RegisterRef> &P);
+  raw_ostream &operator<<(raw_ostream &OS, const Print<NodeId> &P);
+  raw_ostream &operator<<(raw_ostream &OS, const Print<NodeAddr<DefNode *>> &P);
+  raw_ostream &operator<<(raw_ostream &OS, const Print<NodeAddr<UseNode *>> &P);
+  raw_ostream &operator<<(raw_ostream &OS,
+                          const Print<NodeAddr<PhiUseNode *>> &P);
+  raw_ostream &operator<<(raw_ostream &OS, const Print<NodeAddr<RefNode *>> &P);
+  raw_ostream &operator<<(raw_ostream &OS, const Print<NodeList> &P);
+  raw_ostream &operator<<(raw_ostream &OS, const Print<NodeSet> &P);
+  raw_ostream &operator<<(raw_ostream &OS, const Print<NodeAddr<PhiNode *>> &P);
+  raw_ostream &operator<<(raw_ostream &OS,
+                          const Print<NodeAddr<StmtNode *>> &P);
+  raw_ostream &operator<<(raw_ostream &OS,
+                          const Print<NodeAddr<InstrNode *>> &P);
+  raw_ostream &operator<<(raw_ostream &OS,
+                          const Print<NodeAddr<BlockNode *>> &P);
+  raw_ostream &operator<<(raw_ostream &OS,
+                          const Print<NodeAddr<FuncNode *>> &P);
+  raw_ostream &operator<<(raw_ostream &OS, const Print<RegisterSet> &P);
+  raw_ostream &operator<<(raw_ostream &OS, const Print<RegisterAggr> &P);
+  raw_ostream &operator<<(raw_ostream &OS,
+                          const Print<DataFlowGraph::DefStack> &P);
+
 } // end namespace rdf
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_HEXAGON_RDFGRAPH_H

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

 namespace rdf {
 
 raw_ostream &operator<< (raw_ostream &OS, const PrintLaneMaskOpt &P) {
   if (!P.Mask.all())
     OS << ':' << PrintLaneMask(P.Mask);
   return OS;
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS, const Print<RegisterRef> &P) {
   auto &TRI = P.G.getTRI();
   if (P.Obj.Reg > 0 && P.Obj.Reg < TRI.getNumRegs())
     OS << TRI.getName(P.Obj.Reg);
   else
     OS << '#' << P.Obj.Reg;
   OS << PrintLaneMaskOpt(P.Obj.Mask);
   return OS;
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS, const Print<NodeId> &P) {
   auto NA = P.G.addr<NodeBase*>(P.Obj);
   uint16_t Attrs = NA.Addr->getAttrs();
   uint16_t Kind = NodeAttrs::kind(Attrs);
   uint16_t Flags = NodeAttrs::flags(Attrs);
   switch (NodeAttrs::type(Attrs)) {
     case NodeAttrs::Code:
       switch (Kind) {

 static void printRefHeader(raw_ostream &OS, const NodeAddr<RefNode*> RA,
                 const DataFlowGraph &G) {
   OS << Print<NodeId>(RA.Id, G) << '<'
      << Print<RegisterRef>(RA.Addr->getRegRef(G), G) << '>';
   if (RA.Addr->getFlags() & NodeAttrs::Fixed)
     OS << '!';
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<DefNode*>> &P) {
   printRefHeader(OS, P.Obj, P.G);
   OS << '(';
   if (NodeId N = P.Obj.Addr->getReachingDef())
     OS << Print<NodeId>(N, P.G);
   OS << ',';
   if (NodeId N = P.Obj.Addr->getReachedDef())
     OS << Print<NodeId>(N, P.G);
   OS << ',';
   if (NodeId N = P.Obj.Addr->getReachedUse())
     OS << Print<NodeId>(N, P.G);
   OS << "):";
   if (NodeId N = P.Obj.Addr->getSibling())
     OS << Print<NodeId>(N, P.G);
   return OS;
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<UseNode*>> &P) {
   printRefHeader(OS, P.Obj, P.G);
   OS << '(';
   if (NodeId N = P.Obj.Addr->getReachingDef())
     OS << Print<NodeId>(N, P.G);
   OS << "):";
   if (NodeId N = P.Obj.Addr->getSibling())
     OS << Print<NodeId>(N, P.G);
   return OS;
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS,
       const Print<NodeAddr<PhiUseNode*>> &P) {
   printRefHeader(OS, P.Obj, P.G);
   OS << '(';
   if (NodeId N = P.Obj.Addr->getReachingDef())
     OS << Print<NodeId>(N, P.G);
   OS << ',';
   if (NodeId N = P.Obj.Addr->getPredecessor())
     OS << Print<NodeId>(N, P.G);
   OS << "):";
   if (NodeId N = P.Obj.Addr->getSibling())
     OS << Print<NodeId>(N, P.G);
   return OS;
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<RefNode*>> &P) {
   switch (P.Obj.Addr->getKind()) {
     case NodeAttrs::Def:
       OS << PrintNode<DefNode*>(P.Obj, P.G);
       break;
     case NodeAttrs::Use:
       if (P.Obj.Addr->getFlags() & NodeAttrs::PhiRef)
         OS << PrintNode<PhiUseNode*>(P.Obj, P.G);
       else
         OS << PrintNode<UseNode*>(P.Obj, P.G);
       break;
   }
   return OS;
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS, const Print<NodeList> &P) {
   unsigned N = P.Obj.size();
   for (auto I : P.Obj) {
     OS << Print<NodeId>(I.Id, P.G);
     if (--N)
       OS << ' ';
   }
   return OS;
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS, const Print<NodeSet> &P) {
   unsigned N = P.Obj.size();
   for (auto I : P.Obj) {
     OS << Print<NodeId>(I, P.G);
     if (--N)
       OS << ' ';
   }
   return OS;
for (NodeAddr<T> A : P.List) {
       if (--N)
         OS << ", ";
     }
     return OS;
   }
 
 } // end anonymous namespace
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<PhiNode*>> &P) {
   OS << Print<NodeId>(P.Obj.Id, P.G) << ": phi ["
      << PrintListV<RefNode*>(P.Obj.Addr->members(P.G), P.G) << ']';
   return OS;
 }
 
-template<>
-raw_ostream &operator<< (raw_ostream &OS,
-      const Print<NodeAddr<StmtNode*>> &P) {
+raw_ostream &operator<<(raw_ostream &OS, const Print<NodeAddr<StmtNode *>> &P) {
   const MachineInstr &MI = *P.Obj.Addr->getCode();
   unsigned Opc = MI.getOpcode();
   OS << Print<NodeId>(P.Obj.Id, P.G) << ": " << P.G.getTII().getName(Opc);
   // Print the target for calls and branches (for readability).
   if (MI.isCall() || MI.isBranch()) {
     MachineInstr::const_mop_iterator T =
           llvm::find_if(MI.operands(),
                         [] (const MachineOperand &Op) -> bool {
                           return Op.isMBB() || Op.isGlobal() || Op.isSymbol();
                         });
     if (T != MI.operands_end()) {
       OS << ' ';
       if (T->isMBB())
         OS << printMBBReference(*T->getMBB());
       else if (T->isGlobal())
         OS << T->getGlobal()->getName();
       else if (T->isSymbol())
         OS << T->getSymbolName();
     }
   }
   OS << " [" << PrintListV<RefNode*>(P.Obj.Addr->members(P.G), P.G) << ']';
   return OS;
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS,
       const Print<NodeAddr<InstrNode*>> &P) {
   switch (P.Obj.Addr->getKind()) {
     case NodeAttrs::Phi:
       OS << PrintNode<PhiNode*>(P.Obj, P.G);
       break;
     case NodeAttrs::Stmt:
       OS << PrintNode<StmtNode*>(P.Obj, P.G);
       break;
     default:
       OS << "instr? " << Print<NodeId>(P.Obj.Id, P.G);
       break;
   }
   return OS;
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS,
       const Print<NodeAddr<BlockNode*>> &P) {
   MachineBasicBlock *BB = P.Obj.Addr->getCode();
   unsigned NP = BB->pred_size();
   std::vector<int> Ns;
   auto PrintBBs = [&OS] (std::vector<int> Ns) -> void {
     unsigned N = Ns.size();
     for (int I : Ns) {
raw_ostream &operator<< (raw_ostream &OS,
   PrintBBs(Ns);
   OS << '\n';
 
   for (auto I : P.Obj.Addr->members(P.G))
     OS << PrintNode<InstrNode*>(I, P.G) << '\n';
   return OS;
 }
 
-template<>
-raw_ostream &operator<< (raw_ostream &OS,
-      const Print<NodeAddr<FuncNode*>> &P) {
+raw_ostream &operator<<(raw_ostream &OS, const Print<NodeAddr<FuncNode *>> &P) {
   OS << "DFG dump:[\n" << Print<NodeId>(P.Obj.Id, P.G) << ": Function: "
      << P.Obj.Addr->getCode()->getName() << '\n';
   for (auto I : P.Obj.Addr->members(P.G))
     OS << PrintNode<BlockNode*>(I, P.G) << '\n';
   OS << "]\n";
   return OS;
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS, const Print<RegisterSet> &P) {
   OS << '{';
   for (auto I : P.Obj)
     OS << ' ' << Print<RegisterRef>(I, P.G);
   OS << " }";
   return OS;
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS, const Print<RegisterAggr> &P) {
   P.Obj.print(OS);
   return OS;
 }
 
-template<>
 raw_ostream &operator<< (raw_ostream &OS,
       const Print<DataFlowGraph::DefStack> &P) {
   for (auto I = P.Obj.top(), E = P.Obj.bottom(); I != E; ) {
     OS << Print<NodeId>(I->Id, P.G)
        << '<' << Print<RegisterRef>(I->Addr->getRegRef(P.G), P.G) << '>';
     I.down();
     if (I != E)
       OS << ' ';

llvm/trunk/lib/Target/Hexagon/RDFLiveness.h

private:
     void traverse(MachineBasicBlock *B, RefMap &LiveIn);
     void emptify(RefMap &M);
 
     std::pair<NodeSet,bool> getAllReachingDefsRecImpl(RegisterRef RefRR,
         NodeAddr<RefNode*> RefA, NodeSet &Visited, const NodeSet &Defs,
         unsigned Nest, unsigned MaxNest);
   };
 
+  raw_ostream &operator<<(raw_ostream &OS, const Print<Liveness::RefMap> &P);
+
 } // end namespace rdf
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_HEXAGON_RDFLIVENESS_H

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

 using namespace rdf;
 
 static cl::opt<unsigned> MaxRecNest("rdf-liveness-max-rec", cl::init(25),
   cl::Hidden, cl::desc("Maximum recursion level"));
 
 namespace llvm {
 namespace rdf {
 
-  template<>
   raw_ostream &operator<< (raw_ostream &OS, const Print<Liveness::RefMap> &P) {
     OS << '{';
     for (auto &I : P.Obj) {
       OS << ' ' << printReg(I.first, &P.G.getTRI()) << '{';
       for (auto J = I.second.begin(), E = I.second.end(); J != E; ) {
         OS << Print<NodeId>(J->first, P.G) << PrintLaneMaskOpt(J->second);
         if (++J != E)
           OS << ',';