[PATCH] D88595: [TableGen] Add not_all_same constraint check

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for working on this @greened!



================
Comment at: llvm/utils/TableGen/CodeGenInstruction.cpp:256
+static SmallVector<ConstraintArg, 8>
+ParseConstraintOperands(const std::string CStr, CGIOperandList &Ops,
+                        Record *Rec) {
----------------
nit: s/CStr/ConstraintString/

I keep reading this as C string, whereas it's actually a `std::string` :)


================
Comment at: llvm/utils/TableGen/CodeGenInstruction.cpp:266
+  while (Start != std::string::npos) {
+    std::string::size_type Delim = CStr.find_first_of(Delims, Start);
+    if (Delim == std::string::npos)
----------------
If you would pass the constraint as a StringRef, then you can use its convenient `split()` method and also use the `trim` method to trim any whitespace.


================
Comment at: llvm/utils/TableGen/CodeGenInstruction.cpp:285
+static bool
+PlaceNotAllSameConstraint(CGIOperandList::ConstraintInfo NewConstraint,
+                          CGIOperandList &Ops) {
----------------
nit: given that it may return true/false based on success/failure, can you rename it to `tryToPlaceNotAllSameConstraint`?


================
Comment at: llvm/utils/TableGen/CodeGenInstruction.cpp:291
+  // be straightforward but unfortunately we don't know if we might see a
+  // constraint in the future.  The tied constraint will go on the source so we
+  // would prefer not to put it there.  In addition, a destination may have an
----------------
Is there a reason it can only be applied to a single operand? I would expect the constraint to be applied to all the operands that are described in the constraint string.
```/// This holds information about one operand of a machine instruction,
/// indicating the register class for register operands, etc.
class MCOperandInfo {
public:

  [...]

  /// The lower 16 bits are used to specify which constraints are set.
  /// The higher 16 bits are used to specify the value of constraints (4 bits
  /// each).
  uint32_t Constraints;
```
That suggests that multiple constraints can be set per operand?


================
Comment at: llvm/utils/TableGen/CodeGenInstruction.cpp:411
+
+  if (Name == "not_all_same") {
+    // not_all_same(op1, op2, ...)
----------------
nit: Maybe pull the semantic checks out into a separate function?


================
Comment at: llvm/utils/TableGen/CodeGenInstruction.cpp:441
+    for (unsigned i = 0; i < ConstraintOps.size(); ++i) {
+      if (ConstraintOps[i] > 3) {
+        PrintFatalError(Rec->getLoc(),
----------------
Should this be `>= Operands.size()` ?


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:187
+      else if (Constraint.isNotAllSame()) {
+        // We have four bits to work with.  Consider the four bits to represent
+        // a set of operands, where a 1 bit means that operand participates in
----------------
This only working for 4 operands doesn't seem very future proof.  I guess you can still redefine that bitfield to take '8bits' info for each constraint, so that there are 3 bytes for the 3 constraints + one byte that specifies which constraints are enabled. New constraints that are added after this one would mean changing the int32_t -> int64_t, but given that after all these years only IsTied and EarlyClobber exist, shows doesn't need to happen very often :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88595