[llvm-dev] Do you accept simple method overload in the SelectionDAG class?

[Alex Susu via llvm-dev]

   Hello.
     I would like to propose (another) small change to the SelectionDAG class. This change 
is proposed at https://reviews.llvm.org/D60052#change-7t38wfUuDjbR where I added via 
overloading 2 new SelectionDAG::getMachineNode() methods that would allow from now on to 
pass four SDValue operands, something not available before. More exactly these methods are:
       MachineSDNode *SelectionDAG::getMachineNode(unsigned Opcode, const SDLoc &dl,
                                                   EVT VT,
                                                   SDValue Op1, SDValue Op2,
                                                   SDValue Op3, SDValue Op4) {
         SDVTList VTs = getVTList(VT);
         SDValue Ops[] = { Op1, Op2, Op3, Op4 };
         return getMachineNode(Opcode, dl, VTs, Ops);
       }

       MachineSDNode *SelectionDAG::getMachineNode(unsigned Opcode, const SDLoc &dl,
                                                   EVT VT1, EVT VT2,
                                                   SDValue Op1, SDValue Op2,
                                                   SDValue Op3, SDValue Op4) {
         SDVTList VTs = getVTList(VT1, VT2);
         SDValue Ops[] = { Op1, Op2, Op3, Op4 };
         return getMachineNode(Opcode, dl, VTs, Ops);
       }

     In case you are familiar with SelectionDAG.cpp, would you accept that I commit these 
2 new SelectionDAG::getMachineNode() methods which allow 4 input operands, Op1 Op2 Op3 
Op4?
    Quentin Colombet and even I more recently think that maybe adding these methods are 
less useful since there are already methods equivalent to them:
      > QC: Hi Alex,[...] This functionality is already covered by the array/list API of 
that method:
      >          MachineSDNode *getMachineNode(unsigned Opcode, const SDLoc &dl, 
ArrayRef<EVT> ResultTys, ArrayRef<SDValue> Ops);
      >          MachineSDNode *getMachineNode(unsigned Opcode, const SDLoc &dl, SDVTList 
VTs, ArrayRef<SDValue> Ops);
      >          You can ask llvm-dev.Personally I don't think the case with 4 arguments 
is occurring often enough that it is worth adding this "overload". I say overload since 
you can already achieve that with the existing API using the lists of ops. (E.g., the 
implementation of the 4 arguments is just a wrapper around the list API.) Now, maybe some 
people are seeing this more than I do, so it may be worth [...]. Up to you!

     Please note however that I overloaded with these 2 new methods because I created 
several MachineSDNode with tied-to constraints, which normally require 3+1 input operands, 
1 more related to the tied-to constraint. More exactly in my back end I write code like 
this in the SelectionDAGISel inherited class:
         SDNode *sub2 = DAG->getMachineNode(
                                             Connex::SUBV_H_TIED, // an instruction with 
tied-to constraints
                                             DL,
                                             TYPE_VECTOR_I16,
                                             MVT::Glue,
                                             SDValue(vload1, 0), // source 1
                                             SDValue(or1, 0), // source 2
                                             SDValue(or1, 0), // tied-to constraint source 
(source 3) for the destination
                                             SDValue(whereeq2, 0) // glue input edge
                                             );
     This SUBV_H_TIED is defined with Tablegen as an instruction with tied-to constraints 
like this:
             class MSA_3R_SPECIAL_DESC_BASE<string instr_asm,
                                            SDPatternOperator OpNode,
                                            RegisterOperand ROWD,
                                            RegisterOperand ROWS = ROWD,
                                            RegisterOperand ROWT = ROWD,
                                            RegisterOperand ROWS_TIED = ROWD,
                                            InstrItinClass itin = NoItinerary> ...
     I can provide more information if you think it is required.



   Thank you,
     Alex