[PATCH] D46356: [TableGen] Emit a fatal error on inconsistencies in resource units vs cycles.

[Clement Courbet via Phabricator via llvm-commits]

courbet added a comment.

In https://reviews.llvm.org/D46356#1085209, @andreadb wrote:

> In https://reviews.llvm.org/D46356#1085183, @courbet wrote:
>
> > Hi Andrea,
> >
> > In https://reviews.llvm.org/D46356#1085164, @andreadb wrote:
> >
> > > I think that your new check is a bit too strict.
> > >  It is okay to have less ResourceCycle elements than resources in ProcResources. It simply means that some resources would implicitly be consumed for 1cy.
> >
> >
> > Yes, I understand that this is the current behaviour. But I think this change shows that relying on this implicit behaviour is way too dangerous as shown by the bugs it has uncovered in MIPS, Exynos (I think), ThunderX, Broadwell, and Silvermont.
> >
> > > Example:
> > > 
> > >   // Reserve A9UnitFP for 2 consecutive cycles.
> > >   def A9Write2V4 : SchedWriteRes<[A9UnitFP, A9UnitAGU]> {
> > >     let Latency = 4;
> > >     let ResourceCycles = [2];
> > > 
> > > 
> > > It is okay not to have an explicit [2,1] here. The A9UnitAGU should get an implicit 1cy.
> >
> > It's semantically similar, but not obvious: someone unfamiliar with the schema could think that this is 1 cycle on A9UnitFP and one on A9UnitAGU. I think it's clearer to spell it out explicitly.
>
>
> My opinion is that the description in TargetSchedule.td is quite clear.
>
>   // Optionally, ResourceCycles indicates the number of cycles the
>   // resource is consumed. Each ResourceCycles item is paired with the
>   // ProcResource item at the same position in its list. Since
>   // ResourceCycles are rarely specialized, the list may be
>   // incomplete. By default, resources are consumed for a single cycle [...]
>


It is, but it clearly does not prevent bugs in practice :)

> you should probably update the comment in TargetSchedule.td (lines [283:292]). Your patch slightly changes the set of constraints on the resource cycles; I think that one of the statements in that paragraph may not say the truth anymore.

Yep, this was done in update 3 of this diff


Repository:
  rL LLVM

https://reviews.llvm.org/D46356