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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
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
----------------
greened wrote:
> greened wrote:
> > 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.
> Thinking about this some more, I'll see if I can lift this restriction.  It certainly would simplify the handling of this constraint since we wouldn't need all the rigmarole around finding a (single) place for it.
> 
> I don't really understand the upper/lower 16 bit thing.  If we have the ability to signal 16 different constraints on an operand, why does the comment talk about four bits of value for each constraint?  That would seem to mean only four constraints are allowed at once.  It's an odd partition of the 32 bits available.
> 
> Thinking about this some more, I'll see if I can lift this restriction. It certainly would simplify the handling of this constraint since we wouldn't need all the rigmarole around finding a (single) place for it.
Thanks! The tests are quite confusing on how it tries to find a suitable operand to place the constraint.

> I don't really understand the upper/lower 16 bit thing. If we have the ability to signal 16 different constraints on an operand, why does the comment talk about four bits of value for each constraint? That would seem to mean only four constraints are allowed at once. It's an odd partition of the 32 bits available.
The partitioning is odd indeed, but with only two constraints I doubt there was never a need to reconsider this.


================
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
----------------
greened wrote:
> greened wrote:
> > 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.  :)
> I like your 8 bit value idea though.  If we think it's important to do sooner rather than later (because near-future ISAs needing this constraint will have more than four operands) perhaps I can make the 8 bit change first and then land this one.  I don't really know what the implications are of changing the constraint layout though.  It's been this way for so long that it makes me nervous to change it.  What do you think?  Important to do now or can it wait?
It should be fine to fix in a follow-up patch as the instructions you're trying to target only have four operands and those will be the only instructions needing the new constraint for now.
That said, from a quick glance `MCInstrDesc::getOperandConstraint` seems to be the only interface directly using it (returning as an `int`), so I think it's worth a try to see if this a one/few line change is sufficient (and if not, to just leave it for a follow-up patch)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88595