[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 reviewer: andreadb.
andreadb added a comment.

Hi Clement,

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.

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.

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.

On the other hand, I totally agree with you that the following case is a bug:

  def BWWriteResGroup138 : SchedWriteRes<[BWPort0,BWPort5,BWPort23]> {
    let Latency = 13;
    let NumMicroOps = 4;
    let ResourceCycles = [1,2,1,7];

Here ResourceCycles has more elements than the number of resources specified by the write (i.e. 4 entries versus 3 resources).

The next example is "well-formed", although semantically incorrect. We should have that 70 is associated to SLMFPDivider. However, strictly speaking, it is not wrong to have only a list of two cycles (instead of three).

  def SLMriteResGroup13 : SchedWriteRes<[SLM_MEC_RSV,SLM_FPC_RSV0,SLMFPDivider]> {
    let Latency = 74;
    let NumMicroOps = 1;
    let ResourceCycles = [1,70];
  }

I am not against introducing/enforcing more constraints. Personally, I would only error if `Cycles.size() > PRVec.size()`, and not if `Cycles.size() != PRVec.size()`.
This is just my opinion though.
I do recognize though that this would help catching bugs similar to the div from the last example.


Repository:
  rL LLVM

https://reviews.llvm.org/D46356