[PATCH] D88385: [TableGen][M68K] (Patch 1/8) Utilities for complex instruction addressing modes: CodeBeads and logical operand helper functions

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/utils/TableGen/CMakeLists.txt:58
   CTagsEmitter.cpp
+  CodeBeadsGen.cpp
   )
----------------
This list was nearly in alphabetical order. Can you put this file in the correct place.


================
Comment at: llvm/utils/TableGen/CodeBeadsGen.cpp:50
+
+  unsigned Length = 192;
+  unsigned Size = 8;
----------------
Where do these numbers come from? Are they specific to 68K?


================
Comment at: llvm/utils/TableGen/CodeBeadsGen.cpp:85
+    for (unsigned p = 0; p < Parts; ++p) {
+      // unsigned Num = BI->getNumBits();
+      unsigned Right = Size * p;
----------------
Remove commented out code?


================
Comment at: llvm/utils/TableGen/CodeBeadsGen.cpp:92
+        unsigned Shift = i % Size;
+        if (BitInit *B = dyn_cast<BitInit>(BI->getBit(i))) {
+          Value |= (uint64_t)B->getValue() << (Shift);
----------------
You can use auto here


================
Comment at: llvm/utils/TableGen/CodeBeadsGen.cpp:97
+                          "Record `" + R->getName() + "', bit 'Beads[" +
+                              std::to_string(i) + "]' is not defined");
+        }
----------------
I think you can use Twine(i) here instead of std::to_string(i).


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:466
+                    });
+    if (LogicalOpList.size() > LogicalOpListSize)
+      LogicalOpListSize = LogicalOpList.size();
----------------
std::max?


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:470
+    auto I = LogicalOpSizeMap.find(LogicalOpList);
+    if (I == LogicalOpSizeMap.end()) {
+      I = LogicalOpSizeMap.insert({LogicalOpList, LogicalOpSizeMap.size()})
----------------
Is the find and if necessary? Won't insert avoid overwriting value if its already in the map?


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:474
+    }
+    InstMap[I->second].push_back(Namespace.str() +
+                                 "::" + Inst->TheDef->getName().str());
----------------
I think you can use (Namespace + "::" + Inst->TheDef->getName()).str().   That will create a Twine for the pieces and the convert it to a std::string at the end. This should be slightly more efficient than concatenating std::strings.


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:497
+      for (i = 0; i < s; ++i) {
+        if (i > 0)
+          OS << ", ";
----------------
Probably don't need to bother avoiding trailing commas in the generated file. The C++ parsing rules allow them


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:518
+      auto &Insts = P.second;
+      llvm::for_each(
+          Insts, [&](const std::string &S) { OS << "  case " << S << ":\n"; });
----------------
I think we should just use a range for loop here. llvm::for_each is discouraged here https://llvm.org/docs/CodingStandards.html#use-range-based-for-loops-wherever-possible


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:561
+          !OpR->isAnonymous()) {
+        LogicalOpTypeList.push_back(Namespace.str() +
+                                    "::OpTypes::" + Op.Rec->getName().str());
----------------
Same comment as earlier about using a Twine here.


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:567
+    }
+    if (LogicalOpTypeList.size() > OpTypeListSize)
+      OpTypeListSize = LogicalOpTypeList.size();
----------------
std::max?


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:570
+
+    auto I = LogicalOpTypeMap.find(LogicalOpTypeList);
+    if (I == LogicalOpTypeMap.end()) {
----------------
I think insert is sufficient.


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:618
+      auto &Insts = P.second;
+      llvm::for_each(
+          Insts, [&](const std::string &S) { OS << "  case " << S << ":\n"; });
----------------
range for


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88385/new/

https://reviews.llvm.org/D88385