<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Nov 28, 2011, at 1:38 PM, Anshuman Dasgupta wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; font-family: Menlo; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><span class="Apple-style-span" style="font-family: monospace; ">Sure, I've attached documentation on the functionality of the DFA. The document presents an overview of how the DFA is constructed and what it does. I will work on a patch to CodeGenerator.html that will describe the DFA generator but this document should let you review the code's functionality.<br></span></span></blockquote></div><br><div>Thanks, that helped.</div><div><br></div><div>How big do these tables get for Hexagon? How many resources? How many DFA states?</div><div><br></div><div><br></div><div><div>+//=- llvm/CodeGen/DFAPacketizer.h - DFA Packetizer for VLIW ---*- C++ -*-=====//</div></div><div><br></div><div>This file needs more concrete, less abstract documentation.  The DFA concept is quite simple, just explain it.</div><div><br></div><div>This class definitely needs to use DenseMap instead of std::map.  How big is your state space?  Would it make sense to use a sparse transition matrix instead?</div><div><br></div><div>I also think you should move some of the methods to a .cpp file. If you can avoid the #includes, that would be great.</div><div><br></div><div>You are building CachedTable on demand. Is that really necessary? How much of the table is built in a typical run? Would it be better/faster to just build the whole thing up front?</div><div><br></div><div>You have 4 table lookups per DFA transition by my count. It seems that 1 is enough. Can the API be improved to allow that?</div><div><br></div><div><br></div><div><div>+++ b/utils/TableGen/DFAPacketizerEmitter.cpp</div></div><div><div>+bool State::canAddInsnClass(Input InsnClass, std::set<Input>& PossibleStates) {</div></div><div><br></div><div>Would a BitVector be better than set<unsigned>? You can still iterate.</div><div><br></div><div><div>+// Format:</div><div>+// DFAStateInputTable[] = pairs of <Input, Transition> for all valid</div><div>+//                        transitions</div><div>+// DFAStateEntryTable[i] = Index of the first entry in DFAStateInputTable for</div><div>+//                         the ith state</div><div><br></div></div><div>Could you get rid of the DFAStateEntryTable by renumbering your states?</div><div><br></div><div>  s -> DFAStateEntryTable[s]</div><div><br></div><div>This would preclude the sparse transition matrix representation, of course.</div><div><br></div><div><br></div><div><div>+      if (!isValidTransition(*SI, j)) {</div><div>+        continue;</div><div>+      }</div><div>+      else {</div><div><br></div></div><div>Don't use else after continue;</div><div><br></div><div><div>+  OS << "const unsigned int " << TargetName << "DFAStateEntryTable[] = {\n";</div><div>+  for (unsigned i = 0; i < states.size(); ++i) {</div><div>+    // Multiply i by 2 since each entry in DFAStateInputTable is a set of</div><div>+    // two numbers</div><div>+    OS << StateEntry[i] * 2 << ", ";</div><div>+  }</div><div><br></div></div><div>How about using "const unsigned DFAStateInputTable[][2]" then?</div><div><br></div><div><div>+      std::set<unsigned> NewStateResources;</div><div>+      //</div><div>+      // If we haven't already created a transition for this input</div><div>+      // and the state can accommodate this InsnClass, create a transition</div><div>+      //</div><div>+      if (!D.getTransition(current, InsnClass) &&</div><div>+          current->canAddInsnClass(InsnClass, NewStateResources)) {</div><div>+        State* NewState = NULL;</div><div>+</div><div>+        //</div><div>+        // If we have seen this state before, then do not create a new state</div><div>+        //</div><div>+        //</div><div>+        std::string key = "";</div><div>+        std::stringstream keyInt;</div><div>+        // We should not generate more than 16^8 states!</div><div>+        const int MaxSize = 8;</div><div>+        for (std::set<Input>::iterator KI = NewStateResources.begin(),</div><div>+               KE = NewStateResources.end(); KI != KE; ++KI) {</div><div>+          keyInt << std::setfill('0') << std::setw(MaxSize)</div><div>+                 << std::hex << *KI << std::dec;</div><div>+        }</div><div>+        key = keyInt.str();</div><div><br></div></div><div>This string conversion is a bit strange. I think this is actually a case where std::map<std::set<unsigned>, ...> makes sense. Other TableGen backends do that.</div><div><br></div><div>Would it be reasonable to limit the number of resources to 64, and just use a uint64_t bit mask instead?</div><div><br></div><div>The Visited map can be a local variable like WorkList, right?</div><div><br></div><div>/jakob</div><div><br></div></body></html>