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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

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 [...]



>> The next example is "well-formed", although semantically incorrect.
> 
> That's the point of the change: it's currently too easy to accidentally write well-formed but incorrect specs.

I agree with your last point. It would definitely force people to be more careful when they write those cycles.

If Simon/other people are happy with it, then I am happy too.

About your patch:
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.



================
Comment at: utils/TableGen/SubtargetEmitter.cpp:1123
+        }
+        // TODO(courbet): check NumMicroOps.
+
----------------
Please remove this TODO (or remove your name from the TODO comment).


Repository:
  rL LLVM

https://reviews.llvm.org/D46356