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

Wed May 12 09:41:15 PDT 2021

dancgr 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
----------------
sdesmalen wrote:
> greened wrote:
> > 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.
> Thanks for looking at this again @greened, I had to swap some pages back in my memory to understand the state of this :)
> 
> Looking at this patch with a slightly fresh view, I realised it may not be strictly necessary to add support for more than one constraint per operand, because the constraints can be sufficiently simplified. Some observations when there are operands with >1 constraint:
> 
> * EarlyClobber is a more strict variant of `not_all_same` in that it specifically requires the `earlyclobber` operand to be allocated a different register from the rest. The `not_all_same` constraint is then already satisfied by the `earlyclobber`, making the constraint redundant. That means for a set of constraints, the following rule applies:
> ```{not_all_same(Dst, Op0, Op1), earlyclobber(Dst)} => {earlyclobber(Dst)}```
> 
> * Tied operands conflict with `not_all_same` in that two operands cannot be the same if one of them must be different. That means the following rule applies:
> ```{IsTied(Dst, Op0), not_all_same(Dst, Op0, Op1, Op2)} => {IsTied(Dst, Op0), not_all_same(Op1, Op2)}```
> The register allocator is still free to choose either Op1 or Op2 to be the same as Dst(==Op0), but not both.
> 
> * For the case where there would only be one remaining register that needs to be different, the following rule applies:
> ```{IsTied(Dst, Op0), not_all_same(Dst, Op0, Op1)} => {IsTied(Dst, Op0), EarlyClobber(Dst)}```
> This would satisfy that Op1 must be different from Dst/Op0. According to the LangRef this case is already supported:
> > It is permitted to tie an input to an “early-clobber” output. In that case, no other input may share the same register as the input tied to the early-clobber (even when the other input has the same value).
> 
> The only case that would not be supported is having multiple overlapping `not_all_same` constraints, but I'm not sure how important it is to support that (as long as TableGen gives an error when that happens).
> 
> Adding support for multiple constraints is better if we want to extend TableGen with new constraints in the future, but if my reasoning above is correct, it may not be required to consistently represent `not_all_same`. I thought it was worth pointing out there is something in between the current patch and major surgery. Curious to hear your thoughts!
@sdesmalen As far as I understood, you are trying to question whether or not we need to add support for multiple constraints on the same operand, correct?

I agree with your statement. I don't think we need not_all_same and early_clobber at the same time, if early_clobber is statisfied, then not_all_same will also be satisfied. And I agree that there is a conflict between not_all_same and tied operands. Then we are left with the possible need of overlapping not_all_same constraints.

The main example is preventing this:

```
vz0 = FDIV_ZERO pz1/z, vz2, vz2 => z0 = FDIV_ZERO p1, z0, z0
```

To support this case we need only:

```
let Constraints = "not_all_same($Zdn, $_Zdn, $Zm)" in {
  // a[i] = p[i] ? b[i] / c[i] : 0
  defm FDIV_ZERO  : I<(outs z:$Zdn), (ins p:$Pg, z:$_Zdn, z:$Zm)>
  // a[i] = p[i] ? b[i] / c[i] : 0
  defm FDIVR_ZERO : I<(outs z:$Zdn), (ins p:$Pg, z:$_Zdn, z:$Zm)>
}
```

For that we will have: The single constraint prevents vz0 and vz2 from being assinged the same register.

``` vz0 = FDIV_ZERO pz1/z, vz2, vz2 => z1 = FDIV_ZERO p1, z0, z0 ```

If that is the case, I think we can change this patch to only add an extra constraint and remove the multiple constraints aspect.

Is there anything that I missed?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88595