[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: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:
> sdesmalen wrote:
> > 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.
> I looked into this and it seems like allowing multiple contraints on an operand is going to require some major surgery.  It will require that the `Constraints` vector in CGIOperandList::OperandInfo` become multi-dimensional, as currently the vector tracks constraints per "sub-operand."
> 
> Maybe that will be ok but I don't yet have a good handle about what the fallout from that would be.
Actully I think a better approach would be to encode multiple constraints in `ConstraintInfo` and then have the generator use that to emit the operand info.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88595