[llvm-commits] [llvm] r91392 - in /llvm/trunk: include/llvm/CodeGen/SelectionDAG.h lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp lib/CodeGen/SelectionDAG/SelectionDAG.cpp lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

[Chris Lattner]

On Dec 14, 2009, at 5:54 PM, Bill Wendling wrote:

> Author: void
> Date: Mon Dec 14 19:54:51 2009
> New Revision: 91392
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=91392&view=rev
> Log:
> Initial work on disabling the scheduler. This is a work in progress, and this
> stuff isn't used just yet.

Ok, cool.  Here are some comments out of order.

> +++ llvm/trunk/include/llvm/CodeGen/SelectionDAG.h Mon Dec 14 19:54:51 2009
> @@ -110,6 +110,46 @@
>   /// SelectionDAG.
>   BumpPtrAllocator Allocator;
> 
> +  /// NodeOrdering - Assigns a "line number" value to each SDNode that
> +  /// corresponds to the "line number" of the original LLVM instruction. This
> +  /// used for turning off scheduling, because we'll forgo the normal scheduling
> +  /// algorithm and output the instructions according to this ordering.
> +  class NodeOrdering {

SelectionDAG.h already has a ton of stuff in it.  Please pull this out into its own SDNodeOrdering.h file. SelectionDAG.h can just forward declare the class.

> +    /// LineNo - The line of the instruction the node corresponds to. A value of
> +    /// `0' means it's not assigned.
> +    unsigned LineNo;

I don't understand how tis is a "line number".  It seems like these are really unique instruction ID's?  Does this have any correspondence at all to source line numbers?

> +    std::map<const SDNode*, unsigned> Order;

This should use DenseMap.

> +  /// NewInst - Tell the ordering object that we're processing a new
> +  /// instruction.
> +  void NewInst() {
> +    if (Ordering)
> +      Ordering->newInst();
> +  }

This should be in SelectionDAGBuilder, not SelectionDAG.  Likewise, the "current instruction" state should be split out from the NodeOrdering class. 




> ++  public:
> +    NodeOrdering() : LineNo(0) {}
> +
> +    void add(const SDNode *Node) {
> +      assert(LineNo && "Invalid line number!");
> +      Order[Node] = LineNo;
> +    }
> +    void remove(const SDNode *Node) {
> +      std::map<const SDNode*, unsigned>::iterator Itr = Order.find(Node);
> +      if (Itr != Order.end())
> +        Order.erase(Itr);
> +    }
> +    void clear() {
> +      Order.clear();
> +      LineNo = 1;
> +    }
> +    unsigned getLineNo(const SDNode *Node) {
> +      unsigned LN = Order[Node];
> +      assert(LN && "Node isn't in ordering map!");
> +      return LN;
> +    }
> +    void newInst() {
> +      ++LineNo;
> +    }

As above, I don't like the style of this API: you're mixing the construction of the datastructure with the storage of it.  SDBuilder should just have the unsigned counter that it maintains, and this class should just be a thin wrapper around a densemap.

> +++ llvm/trunk/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp Mon Dec 14 19:54:51 2009
> @@ -20,10 +20,16 @@
> #include "llvm/Target/TargetInstrInfo.h"
> #include "llvm/Target/TargetRegisterInfo.h"
> #include "llvm/Target/TargetSubtarget.h"
> +#include "llvm/Support/CommandLine.h"
> #include "llvm/Support/Debug.h"
> #include "llvm/Support/raw_ostream.h"
> using namespace llvm;
> 
> +cl::opt<bool>
> +DisableInstScheduling("disable-inst-scheduling",
> +                      cl::init(false),
> +                      cl::desc("Disable instruction scheduling"));

This be in TargetOptions.h, not a command line option.

> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp Mon Dec 14 19:54:51 2009@@ -778,8 +795,13 @@
> @@ -877,14 +904,17 @@
>   ID.AddPointer(&Val);
>   void *IP = 0;
>   SDNode *N = NULL;
> -  if ((N = CSEMap.FindNodeOrInsertPos(ID, IP)))
> +  if ((N = CSEMap.FindNodeOrInsertPos(ID, IP))) {
> +    if (Ordering) Ordering->add(N);
>     if (!VT.isVector())
>       return SDValue(N, 0);
> +  }
>   if (!N) {
>     N = NodeAllocator.Allocate<ConstantSDNode>();
>     new (N) ConstantSDNode(isT, &Val, EltVT);
>     CSEMap.InsertNode(N, IP);
>     AllNodes.push_back(N);
> +    if (Ordering) Ordering->add(N);

I don't think that this is the right layer to do this at.  The "Ordering" of nodes is only defined at Builder time, not in general when instructions are randomly created by other parts of SD machinery.  The various calls to Ordering->add should only happen from the builder.

OTOH, calls to *remove* a node from the ordering *should* happen from the common SD code when the node is about to be deleted.

> +void SelectionDAG::NodeOrdering::dump() const {
> +}

Please implement or remove this.

-Chris