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

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.

> Same here:
> 
>   defm : ZnWriteResPair<WriteIMul, [ZnALU1, ZnMultiplier], 4>;
> 
> 
> We shouldn't have to explicitly pass a ResourceCycles to [1,1]. IMHO That should be the implied default.

That's Simon's point in him comment. Agreed, I'm going to apply the same approach.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D46356