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

[David Greene via Phabricator via llvm-commits]

greened added inline comments.


================
Comment at: llvm/utils/TableGen/CodeGenInstruction.cpp:256
+static SmallVector<ConstraintArg, 8>
+ParseConstraintOperands(const std::string CStr, CGIOperandList &Ops,
+                        Record *Rec) {
----------------
sdesmalen wrote:
> nit: s/CStr/ConstraintString/
> 
> I keep reading this as C string, whereas it's actually a `std::string` :)
I was following existing convention but I agree with you!


================
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
----------------
sdesmalen wrote:
> 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?
TableGen will not allow multiple constraints on an operand.  There are checks and aborts for that.  Perhaps that restriction can be lifted but that's a whole other can of worms.


================
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
----------------
sdesmalen wrote:
> 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 :)
Again, I think that may be a fine change as a follow-up but I'd rather not conflate it with this change.  This change is large enough already.  :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88595