<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Dec 17, 2009, at 11:38 AM, Chris Lattner wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>On Dec 14, 2009, at 5:54 PM, Bill Wendling wrote:<br><br><blockquote type="cite">Author: void<br></blockquote><blockquote type="cite">Date: Mon Dec 14 19:54:51 2009<br></blockquote><blockquote type="cite">New Revision: 91392<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">URL: <a href="http://llvm.org/viewvc/llvm-project?rev=91392&view=rev">http://llvm.org/viewvc/llvm-project?rev=91392&view=rev</a><br></blockquote><blockquote type="cite">Log:<br></blockquote><blockquote type="cite">Initial work on disabling the scheduler. This is a work in progress, and this<br></blockquote><blockquote type="cite">stuff isn't used just yet.<br></blockquote><br>Ok, cool. Here are some comments out of order.<br><br><blockquote type="cite">+++ llvm/trunk/include/llvm/CodeGen/SelectionDAG.h Mon Dec 14 19:54:51 2009<br></blockquote><blockquote type="cite">@@ -110,6 +110,46 @@<br></blockquote><blockquote type="cite"> /// SelectionDAG.<br></blockquote><blockquote type="cite"> BumpPtrAllocator Allocator;<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">+ /// NodeOrdering - Assigns a "line number" value to each SDNode that<br></blockquote><blockquote type="cite">+ /// corresponds to the "line number" of the original LLVM instruction. This<br></blockquote><blockquote type="cite">+ /// used for turning off scheduling, because we'll forgo the normal scheduling<br></blockquote><blockquote type="cite">+ /// algorithm and output the instructions according to this ordering.<br></blockquote><blockquote type="cite">+ class NodeOrdering {<br></blockquote><br>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.<br><br></div></blockquote><div>Sure.</div><br><blockquote type="cite"><div><blockquote type="cite">+ /// LineNo - The line of the instruction the node corresponds to. A value of<br></blockquote><blockquote type="cite">+ /// `0' means it's not assigned.<br></blockquote><blockquote type="cite">+ unsigned LineNo;<br></blockquote><br>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?<br><br></div></blockquote><div>Nope. :-) They're just unique IDs. I'll change the name to make it clearer.</div><br><blockquote type="cite"><div><blockquote type="cite">+ std::map<const SDNode*, unsigned> Order;<br></blockquote><br>This should use DenseMap.<br><br></div></blockquote><div>Okay.</div><br><blockquote type="cite"><div><blockquote type="cite">+ /// NewInst - Tell the ordering object that we're processing a new<br></blockquote><blockquote type="cite">+ /// instruction.<br></blockquote><blockquote type="cite">+ void NewInst() {<br></blockquote><blockquote type="cite">+ if (Ordering)<br></blockquote><blockquote type="cite">+ Ordering->newInst();<br></blockquote><blockquote type="cite">+ }<br></blockquote><br>This should be in SelectionDAGBuilder, not SelectionDAG. Likewise, the "current instruction" state should be split out from the NodeOrdering class. <br><br><blockquote type="cite">++ public:<br></blockquote><blockquote type="cite">+ NodeOrdering() : LineNo(0) {}<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ void add(const SDNode *Node) {<br></blockquote><blockquote type="cite">+ assert(LineNo && "Invalid line number!");<br></blockquote><blockquote type="cite">+ Order[Node] = LineNo;<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite">+ void remove(const SDNode *Node) {<br></blockquote><blockquote type="cite">+ std::map<const SDNode*, unsigned>::iterator Itr = Order.find(Node);<br></blockquote><blockquote type="cite">+ if (Itr != Order.end())<br></blockquote><blockquote type="cite">+ Order.erase(Itr);<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite">+ void clear() {<br></blockquote><blockquote type="cite">+ Order.clear();<br></blockquote><blockquote type="cite">+ LineNo = 1;<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite">+ unsigned getLineNo(const SDNode *Node) {<br></blockquote><blockquote type="cite">+ unsigned LN = Order[Node];<br></blockquote><blockquote type="cite">+ assert(LN && "Node isn't in ordering map!");<br></blockquote><blockquote type="cite">+ return LN;<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite">+ void newInst() {<br></blockquote><blockquote type="cite">+ ++LineNo;<br></blockquote><blockquote type="cite">+ }<br></blockquote><br>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.<br><br><blockquote type="cite">+++ llvm/trunk/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp Mon Dec 14 19:54:51 2009<br></blockquote><blockquote type="cite">@@ -20,10 +20,16 @@<br></blockquote><blockquote type="cite">#include "llvm/Target/TargetInstrInfo.h"<br></blockquote><blockquote type="cite">#include "llvm/Target/TargetRegisterInfo.h"<br></blockquote><blockquote type="cite">#include "llvm/Target/TargetSubtarget.h"<br></blockquote><blockquote type="cite">+#include "llvm/Support/CommandLine.h"<br></blockquote><blockquote type="cite">#include "llvm/Support/Debug.h"<br></blockquote><blockquote type="cite">#include "llvm/Support/raw_ostream.h"<br></blockquote><blockquote type="cite">using namespace llvm;<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">+cl::opt<bool><br></blockquote><blockquote type="cite">+DisableInstScheduling("disable-inst-scheduling",<br></blockquote><blockquote type="cite">+ cl::init(false),<br></blockquote><blockquote type="cite">+ cl::desc("Disable instruction scheduling"));<br></blockquote><br>This be in TargetOptions.h, not a command line option.<br><br><blockquote type="cite">+++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp Mon Dec 14 19:54:51 2009@@ -778,8 +795,13 @@<br></blockquote><blockquote type="cite">@@ -877,14 +904,17 @@<br></blockquote><blockquote type="cite"> ID.AddPointer(&Val);<br></blockquote><blockquote type="cite"> void *IP = 0;<br></blockquote><blockquote type="cite"> SDNode *N = NULL;<br></blockquote><blockquote type="cite">- if ((N = CSEMap.FindNodeOrInsertPos(ID, IP)))<br></blockquote><blockquote type="cite">+ if ((N = CSEMap.FindNodeOrInsertPos(ID, IP))) {<br></blockquote><blockquote type="cite">+ if (Ordering) Ordering->add(N);<br></blockquote><blockquote type="cite"> if (!VT.isVector())<br></blockquote><blockquote type="cite"> return SDValue(N, 0);<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite"> if (!N) {<br></blockquote><blockquote type="cite"> N = NodeAllocator.Allocate<ConstantSDNode>();<br></blockquote><blockquote type="cite"> new (N) ConstantSDNode(isT, &Val, EltVT);<br></blockquote><blockquote type="cite"> CSEMap.InsertNode(N, IP);<br></blockquote><blockquote type="cite"> AllNodes.push_back(N);<br></blockquote><blockquote type="cite">+ if (Ordering) Ordering->add(N);<br></blockquote><br>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.<br><br></div></blockquote><blockquote type="cite"><div>OTOH, calls to *remove* a node from the ordering *should* happen from the common SD code when the node is about to be deleted.<br><font class="Apple-style-span" color="#006312"><br></font></div></blockquote>Hmm...I was hoping to make this a very low-level thing so that the higher levels wouldn't even know about it. There may be a problem because node creation & modification doesn't stop at the builder, but could happen when lowering to target nodes. I'll look into it, though.</div><div><br></div><div>-bw</div><div><br></div></body></html>