[llvm-commits] [PATCH] TableGen backend support to express relations between instruction

[Jakob Stoklund Olesen]

On Oct 11, 2012, at 8:35 AM, Jyotsna Verma <jverma at codeaurora.org> wrote:

> Hello Jakob/Sean,
> 
> Here is the updated patch which addresses your review comments.  Please take
> a look and let me know if you have any other suggestions.

Please use uint16_t for opcodes instead of 'short' everywhere.

+typedef std::map<std::string, std::vector<Record*> > InstrRelMapTy;

It's not clear why you're keying this map on instruction names instead of using the much more efficient DenseMap<Record*,…>.

+// Compare operator used by RowInstrMap to order keys that are vectors of Init
+// pointers.
+struct InstrMapComp {
+  bool operator() (const std::vector<Init*>& lhs,
+                   const std::vector<Init*>& rhs) const {

Why is this needed?

+void MapTableEmitter::buildRowInstrMap(raw_ostream &OS) {

What's the raw_ostream for?

+    RowInstrMapTy::iterator II, IE;
+    II =  RowInstrMap.find(KeyValue);
+    IE = RowInstrMap.end();
+    if (II == IE) {
+      InstrList.push_back(CurInstr);
+      std::pair<std::vector<Init*>, std::vector<Record*> >
+                                              KeyValuePair(KeyValue, InstrList);
+      RowInstrMap.insert(KeyValuePair);
+    }
+    else {
+      II->second.push_back(CurInstr);
+    }

RowInstrMap[KeyValue].push_back(CurInstr);

+//===----------------------------------------------------------------------===//
+// Output two-level relation tables.
+

I'm rather skeptical about the compile time improvements you can get with this compared to a simple binary search.

Please save this for a later patch, and include some compile time measurements so we know it is worth the extra complexity.

+  OS << "short "<<InstrMapDesc.getName() << "Table[]["<< NumCol+1 << "] = {\n";

Tables are constant.

Please emit just a single function per mapping. The tables can be local to the function, and the binary search algorithm should be duplicated so it doesn't need to deal with dynamic row sizes.

/jakob