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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/include/llvm/Target/Target.td:635
 
+  /// UseLogicalOperandMappings - If set, several helper functions would be
+  /// generated for this instruction to map properties like index, type of
----------------
would -> will


================
Comment at: llvm/include/llvm/Target/Target.td:636-637
+  /// UseLogicalOperandMappings - If set, several helper functions would be
+  /// generated for this instruction to map properties like index, type of
+  /// its logical operands to corresponding MI operands.
+  bit UseLogicalOperandMappings = false;
----------------
Reading this I don't actually understand what it does. How can I map a type to an operand of an MI?


================
Comment at: llvm/utils/TableGen/CodeBeadsGen.cpp:8
+//===----------------------------------------------------------------------===//
+// CodeBeads are annotations that can be used to represent non-trivial
+// instruction variants. More specifically, complex addressing modes appear in
----------------
I don't understand where the name "code bead" comes from. There's no reference to it in the GCC source, nor on the internet in general that I can obviously find, so it sounds to me like something you've invented yourself. Can we please stick to more standard terminology for things? Bead is meaningless.


================
Comment at: llvm/utils/TableGen/CodeBeadsGen.cpp:95
+
+      uint64_t Value = 0;
+      for (unsigned i = Right; i != Left; ++i) {
----------------
uint8_t?


================
Comment at: llvm/utils/TableGen/CodeBeadsGen.cpp:118
+  // Emit initial function code
+  OS << "  return (const uint8_t *)InstBits[Opcode];\n"
+     << "}\n\n";
----------------
Array decay should render the cast unnecessary?


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:491-492
+      OS << "   {";
+      int i, s = Row.size();
+      for (i = 0; i < s; ++i) {
+        OS << Row[i] << ", ";
----------------



================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:474
+    }
+    InstMap[I->second].push_back(Namespace.str() +
+                                 "::" + Inst->TheDef->getName().str());
----------------
craig.topper wrote:
> 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.
Still not done; the std::string construction for Namespace is not necessary.


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

https://reviews.llvm.org/D88385