[PATCH] D41446: [TableGen][AsmMatcherEmitter] Generate assembler checks for tied operands

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks Sander, I left some comments.

This is needed for SVE, because some instructions force 2 registers to be equal for certain versions of the instruction. e.g.

  add z1, z2, z3 # OK
  add z1, p0/m, z2, z3 # Not OK, for predicated add the result reg must be equal to the first data src reg.

Adding Oliver as well, maybe you have some thoughts on the error handling in this case. Is there another way to handle this case?



================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:531
+  /// from 0).
+  std::unordered_set<std::pair<unsigned,unsigned>> AsmOperandEqualityContraints;
+
----------------
Do we need a set here? It seems like we could just used a SmallVector instead. It looks like a set is used to avoid adding duplicated constraints for an instruction, right? 

But the `tied` constraint only gets added to one of the 2 operands (I think), see CodeGenInstructions::ParseConstraint.

Also, those constraints are referred to as tied elsewhere. `AsmOperandTiedContraints` would be a more consistent name I think


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:910
+static Optional<size_t>
+getAsmOperandIdx(SmallVectorImpl<MatchableInfo::AsmOperand> &AsmOperands,
+                 std::string Name) {
----------------
Can AsmOperands be const?


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:914
+  const auto Pos =
+      std::find_if(std::begin(AsmOperands), std::end(AsmOperands),
+                   [&SymbolicName](const MatchableInfo::AsmOperand &A) {
----------------
Why not AsmOperands.begin(), AsmOperands.end()?


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:919
+
+  if (Pos == std::end(AsmOperands))
+    return Optional<size_t>();
----------------
Why not AsmOperands.end()?


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:922
+
+  return Optional<size_t>(std::distance(std::begin(AsmOperands), Pos));
+}
----------------
Why not AsmOperands.begin()?


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:988
+
+    auto LHSIdx = getAsmOperandIdx(AsmOperands, CGIOp.Name);
+    // Skipping operands with constraint but no reference in the
----------------
For relatively short type names, using explicit types slightly improves readability here and elsewhere in this patch


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:1007
+      // pair {A,B} and {B,A} the same
+      auto AddMnemonicIdx = (size_t) HasMnemonicFirst;
+      AsmOperandEqualityContraints.emplace(
----------------
Make type here size_t.


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:2911
 
+static void emitAsmOperandConstraints(CodeGenTarget &Target,
+                                      AsmMatcherInfo &Info,
----------------
Please use Tied in the name here and throughout the function to make it clear that this only deals with `Tied` constraints.


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:2916
+  raw_string_ostream TmpOS(Buf);
+  TmpOS << "static const unsigned DCL[][3] =\n";
+  TmpOS << "  {\n";
----------------
Please use something more descriptive than DCL.


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:2934
+
+    for (const auto &x : Constraints) {
+      TableEmpty = false;
----------------
Use something more descriptive than `x`.


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:2957
+  OS << "  const unsigned Opcode = Inst.getOpcode();\n";
+  OS << "  const unsigned SearchValue[3] = {Opcode, 0, 0};\n";
+  OS << "  const auto Range =\n";
----------------
Having a struct for the SearchValue would better explain what the 3 fields are supposed to be.


https://reviews.llvm.org/D41446