[PATCH] D134073: [TableGen] Add useDeprecatedPositionallyEncodedOperands option.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/Target/Target.td:1052
+  // to bit-field variables both by name and by position. Support for matching
+  // by position is DEPRECATED, and WILL BE REMOVED. Positional matching is
+  // confusing to use, and makes it very easy to accidentally write buggy
----------------
Perhaps use a `TODO` as a minder that the support will be removed.


================
Comment at: llvm/test/TableGen/InsufficientPositionalOperands.td:27
 
-// CHECK: Too few operands in record foo (no match for variable rs)
-  let OutOperandList = (outs Regs:$xd);
+// CHECK: No operand named rs in record foo
+  let OutOperandList = (outs Regs:$rd);
----------------
Add `CHECK-NEXT: note: Dumping record for previous error:` then use `--implicit-check-not=error:`

Q: by changing `$xd`, do we lose test coverage by not testing an undefined operand name?


================
Comment at: llvm/utils/TableGen/CodeEmitterGen.cpp:53
                                             CodeGenTarget &Target);
-  void AddCodeToMergeInOperand(Record *R, BitsInit *BI,
-                               const std::string &VarName,
-                               unsigned &NumberedOp,
+  bool AddCodeToMergeInOperand(Record *R, BitsInit *BI,
+                               const std::string &VarName, unsigned &NumberedOp,
----------------
`addCodeToMergeInOperand` now that the signature has changed


================
Comment at: llvm/utils/TableGen/CodeEmitterGen.cpp:119
   } else {
+    // Fall back to positional lookup. By default, we now disable positional
+    // lookup (and print an error, below), but even so, we'll do the lookup to
----------------
Does this need a TODO that this branch will be removed?


================
Comment at: llvm/utils/TableGen/CodeEmitterGen.cpp:129
+            (!NamedOpIndices.empty() &&
+             NamedOpIndices.count(
+                 CGI.Operands.getSubOperandNumber(NumberedOp).first)))) {
----------------
Drop the change to this code block


================
Comment at: llvm/utils/TableGen/CodeEmitterGen.cpp:157
+        << " with useDeprecatedPositionallyEncodedOperands=true)\n";
+      PrintError(R, E);
+      Success = false;
----------------
Since `PrintError` takes a Twine. You may remove `E` and `S` and just construct the long Twine as an argument


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134073