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

[courbet]

Summary

See PR37310.

For targets I'm not familiar with, I've automatically made the "default to 1 for each resource" behaviour explicit in the td files.
For more obvious cases, I've ventured a fix.

Some notes:

• The change to X86ScheduleBtVer2.td is an NFC, it just makes values more explicit.
• Exynos is especially fishy.
• AArch64SchedThunderX2T99.td had some truncated entries. If I understand correctly, the person who wrote that interpreted the ResourceCycle as a range. I made the decision to use the upper/lower bound for consistency with the 'Latency' value. I'm sure there is a better choice.
• MIPS has the same issue for DIVs.

[courbet]

Fix some MIPS scheduling info revealed by the checks.

[RKSimon]

Not sure if I want to have to specify resource cycles every time - we should be able to assume a default=1, same for latency and micro-ops. This helps reduce bulkiness of the models considerably. In fact I might go further and maybe suggest a debug warning that values set to the default value could be removed?

But I'd definitely happy with the idea that if ResourceCycles are set it must be equal in size to ResourceUnits, (and similar ambiguities).

[andreadb]

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.

[courbet]

• revert changes to btver2, use a saner default.

[courbet]

In D46356#1085159, @RKSimon wrote:

Not sure if I want to have to specify resource cycles every time - we should be able to assume a default=1, same for latency and micro-ops. This helps reduce bulkiness of the models considerably. In fact I might go further and maybe suggest a debug warning that values set to the default value could be removed?

OK, I've changed list<int> Res = [1] to list<int> Res = [] in the definition of JWriteResFpuPair, and added a condition so that the folded version avoids the concat when relying in the default.

[courbet]

Hi Andrea,

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

[courbet]

• Allow fully implicit ResourceCycles in ZnXXXPair model. as per Andrea's comment.
• Update TargetSchedule.td documentation.

[courbet]

• Update llvm-mca SLM tests.

[andreadb]

In D46356#1085183, @courbet wrote:

Hi Andrea,

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

utils/TableGen/SubtargetEmitter.cpp

1123 ↗

(On Diff #144863)

Please remove this TODO (or remove your name from the TODO comment).

[courbet]

In D46356#1085209, @andreadb wrote:

In D46356#1085183, @courbet wrote:

Hi Andrea,

In 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

[courbet]

lib/Target/Mips/MipsScheduleGeneric.td

77 ↗

(On Diff #144869)

@sdardis Can you have a look at this one ? Thanks.

[sdardis]

lib/Target/Mips/MipsScheduleGeneric.td

77 ↗

(On Diff #144869)

Bug. The divide unit in the generic model is supposed to be separate from the ALU. It's a hold-over from when I developed the generic model, as I cribbed it from the P5600 model with has two units working on the divide according to the model.

This change is correct.

82 ↗

(On Diff #144869)

As is this.

[RKSimon]

Can you pull out the SLM / BW fixes into their own commits please?

lib/Target/X86/X86SchedBroadwell.td

1657 ↗

(On Diff #144869)

This fix can be committed now in its own commit.

lib/Target/X86/X86ScheduleSLM.td

393 ↗

(On Diff #144869)

This fix (and the llvm-mca test change) can be committed in its own commit.

[courbet]

lib/Target/Mips/MipsScheduleGeneric.td

77 ↗

(On Diff #144869)

Thanks !

lib/Target/X86/X86SchedBroadwell.td

1657 ↗

(On Diff #144869)

Will do.

[courbet]

Submit MIPS change, rebase.

[courbet]

Submit SLM+BDW fixes and rebase.

[javed.absar]

Thanks for this check.
If ResourceCycles is not specified then it should default to 1 . I believe most people write with that interpretation.
If ResourceCycles is specified for at least one resource, it makes sense from correctness POV that it be specified for all (even though it might get verbose at time).

[courbet]

Rebase.

[andreadb]

Thanks Clement,

It looks good to me now. I only have one minor nit (see below).

utils/TableGen/SubtargetEmitter.cpp

1123 ↗

(On Diff #144863)

This comment should be modified or removed.

[courbet]

Get rid of the FIXME to remove the default ResourceCyles.

[RKSimon]

I think it'd be clearer if we could keep Btver2/Znver1 to the Res = [1] default tbh

[courbet]

Thanks.

@andreadb, @RKSimon, @javed.absar: I'll submit the X86 and A9 changes as they are safe.

I'll wait for confirmation for Exynos and Thunder for the rest.

[courbet]

In D46356#1085328, @RKSimon wrote:

I think it'd be clearer if we could keep Btver2/Znver1 to the Res = [1] default tbh

Hm, not sure how I can do that without triggering the check for cases when the user erroneously specified [1] when they wanted to use the default (now []). Did I miss a possibility ?
This makes is no difference for the user, but allows more checking. Best of both worlds ? :)

[javed.absar]

In D46356#1085333, @courbet wrote:

In D46356#1085328, @RKSimon wrote:

I think it'd be clearer if we could keep Btver2/Znver1 to the Res = [1] default tbh

Hm, not sure how I can do that without triggering the check for cases when the user erroneously specified [1] when they wanted to use the default (now []). Did I miss a possibility ?
This makes is no difference for the user, but allows more checking. Best of both worlds ? :)

I am bit confused on - "user erroneously specified [1] when they wanted to use the default ". But the default was always [1]

[RKSimon]

In D46356#1085333, @courbet wrote:

In D46356#1085328, @RKSimon wrote:

I think it'd be clearer if we could keep Btver2/Znver1 to the Res = [1] default tbh

Hm, not sure how I can do that without triggering the check for cases when the user erroneously specified [1] when they wanted to use the default (now []). Did I miss a possibility ?
This makes is no difference for the user, but allows more checking. Best of both worlds ? :)

Which cases fail?

[courbet]

In D46356#1085339, @javed.absar wrote:

In D46356#1085333, @courbet wrote:

In D46356#1085328, @RKSimon wrote:

I think it'd be clearer if we could keep Btver2/Znver1 to the Res = [1] default tbh

Hm, not sure how I can do that without triggering the check for cases when the user erroneously specified [1] when they wanted to use the default (now []). Did I miss a possibility ?
This makes is no difference for the user, but allows more checking. Best of both worlds ? :)

I am bit confused on - "user erroneously specified [1] when they wanted to use the default ". But the default was always [1]

Now that we're checking what the user wrote, we want the following cases to be valid:

def : WriteRes<SchedRW, [P0, P1]> {
    let ResourceCycles = [1, 1];  // explicit
}

def : WriteRes<SchedRW, [P0, P1]> {
    let ResourceCycles = [];  // explicitly use default, same as let ResourceCycles = [1,1]
}

def : WriteRes<SchedRW, [P0, P1]> {
    // implicitly use default, same as let ResourceCycles = [1,1]
}

But this one to be invalid:

def : WriteRes<SchedRW, [P0, P1]> {
    let ResourceCycles = [1];  // Did the user expect [1,1]; or did they make a mistake ? Throw an error.
}

In this specific case we want:

defm : ZnWriteResPair<WriteIMul,   [ZnALU1, ZnMultiplier], 4>;            // (A) ok, [1,1]
defm : ZnWriteResPair<WriteIMul,   [ZnALU1, ZnMultiplier], 4, [1,1]>;  // (B) ok, [1,1]
defm : ZnWriteResPair<WriteIMul,   [ZnALU1, ZnMultiplier], 4, [1]>;     // (C) Error, not sure if this is a mistake or not.

If I keep the default Res = [1] in the definition of ZnWriteResPair, I cannot make a difference between (A) and (C). Using the the default Res = [] allows making the difference and is consistent with what WriteRes uses as a default.

Am I making more sense ?

[javed.absar]

Ah! OK, I see. Thanks for the clarification.

[RKSimon]

In D46356#1085378, @javed.absar wrote:

Ah! OK, I see. Thanks for the clarification.

Yup - me too! LGTM if @javed.absar is happy as well.

[evandro]

lib/Target/AArch64/AArch64SchedExynosM3.td

307 ↗

(On Diff #144883)

Please, make this [8, 8]...

311 ↗

(On Diff #144883)

[13, 13]...

315 ↗

(On Diff #144883)

[19, 19]...

319 ↗

(On Diff #144883)

[26, 26]...

391 ↗

(On Diff #144883)

[6, 6, 6]...

411 ↗

(On Diff #144883)

[6, 1, 1, 6, 1]...

419 ↗

(On Diff #144883)

[6, 1, 1, 6, 1, 1]...

425 ↗

(On Diff #144883)

[6, 6, 6, 6]...

434 ↗

(On Diff #144883)

[1, 3, 1, 3]...

442 ↗

(On Diff #144883)

[1, 3, 1, 3, 1, 3]...

451 ↗

(On Diff #144883)

[1, 3, 3, 1, 3, 1, 3]...

462 ↗

(On Diff #144883)

[1, 3, 3, 1, 3, 1, 3, 1, 3]...

469 ↗

(On Diff #144883)

[1, 3, 3, 1, 3]...

[evandro]

lib/Target/AArch64/AArch64SchedExynosM3.td

480 ↗

(On Diff #144883)

[1, 3, 3, 1, 3, 1, 3, 1, 3].

[courbet]

Submit NFC changes to ARM and AMD, rebase.

[courbet]

lib/Target/AArch64/AArch64SchedExynosM3.td

307 ↗

(On Diff #144883)

Note that the two following definitions are strictly equivalent:

def M3WriteNEONV   : SchedWriteRes<[M3UnitFDIV, M3UnitFDIV]>  {
  let Latency = 7;
  let NumMicroOps = 1;
  let ResourceCycles = [8,8];
}

def M3WriteNEONV   : SchedWriteRes<[M3UnitFDIV]>  {
  let Latency = 7;
  let NumMicroOps = 1;
  let ResourceCycles = [16];
}

What's the semantics that you're trying to express by splitting into 8*M3UnitFDIV + 8*M3UnitFDIV vs 16*M3UnitFDIV ? The SubtargetEmitter is destroying these semantics anyway.

For reference before this change the definition is equivalent to:

def M3WriteNEONV   : SchedWriteRes<[M3UnitFDIV]>  {
  let Latency = 7;
  let NumMicroOps = 1;
  let ResourceCycles = [9];
}

[evandro]

lib/Target/AArch64/AArch64SchedExynosM3.td

307 ↗

(On Diff #144883)

There are two FDIV units in M3:

def M3UnitFDIV : ProcResGroup<[M3UnitFDIV0, M3UnitFDIV1]>;

[courbet]

lib/Target/AArch64/AArch64SchedExynosM3.td

307 ↗

(On Diff #144883)

Yes indeed, but this is already covered by the M3UnitFDIV referencing these two.

I'm not familiar with Exynos, what's the semantics that you're trying to express here ?

As written before this change, M3WriteNEONV dispatches one Uop that writes its output with latency 7 and uses either of {M3UnitFDIV0, M3UnitFDIV1} for 9 cycles.
After that change, whatever way ([8,8] or [16]) we write it, M3WriteNEONV dispatches one Uop that writes its output with latency 7 and uses either of {M3UnitFDIV0, M3UnitFDIV1} for 16 cycles.

Maybe that's the case, but since that sounds weird, I just want to confirm.

[javed.absar]

Does FDIV use both the resources for 8 cycles? Writing [8,8] would imply that.

[evandro]

In D46356#1086531, @javed.absar wrote:

Does FDIV use both the resources for 8 cycles? Writing [8,8] would imply that.

For FDIVv4f32, yes.

[evandro]

lib/Target/AArch64/AArch64SchedExynosM3.td

307 ↗

(On Diff #144883)

I mean that with M3WriteNEONV, for FDIVv4f32, one uop is dispatched to both [M3UnitFDIV0, M3UnitFDIV1] (don't ask me how this is done inside the machine) and each used up for 8 cycles.

311 ↗

(On Diff #144883)

And here I mean that with M3WriteNEONV, for FDIVv2f64, one uop is dispatched to both [M3UnitFDIV0, M3UnitFDIV1] and each used up for 13 cycles.

[courbet]

• [ExynosM3] Fix scheduling info.

[courbet]

@evandro Do you have benchmarks to evaluate the impact of this change ?

lib/Target/AArch64/AArch64SchedExynosM3.td

307 ↗

(On Diff #144883)

Thanks for the explanation. So you wanted to write:

def M3WriteNEONV   : SchedWriteRes<[M3UnitFDIV0, M3UnitFDIV1]>  {
  let Latency = 7;
  let NumMicroOps = 2;
  let ResourceCycles = [8,8];
}

I've updated the diff with that.

[courbet]

lib/Target/AArch64/AArch64SchedExynosM3.td

376 ↗

(On Diff #145165)

This still feels weirdly asymetric. Is that correct ?

[evandro]

In D46356#1087529, @courbet wrote:

@evandro Do you have benchmarks to evaluate the impact of this change ?

Yes. They're not done yet. Will let you know when I get the results.

Thank you.

lib/Target/AArch64/AArch64SchedExynosM3.td

376 ↗

(On Diff #145165)

No, it should be [5, 5], thank you.

307 ↗

(On Diff #144883)

Thank you.

311 ↗

(On Diff #144883)

Err, I meant M3WriteNEONW here. Thank you.

[courbet]

Fix a last exynos M3 entry.

[courbet]

In D46356#1087868, @evandro wrote:

In D46356#1087529, @courbet wrote:

@evandro Do you have benchmarks to evaluate the impact of this change ?

Yes. They're not done yet. Will let you know when I get the results.

Any news ?
Thanks.

[evandro]

In D46356#1089483, @courbet wrote:

@evandro Do you have benchmarks to evaluate the impact of this change ?

Yes. They're not done yet. Will let you know when I get the results.

Any news ?

Sorry, they slipped my mind. I see a minor improvement overall, especially in FP workloads, with a few minor regressions.

Thank you.

[courbet]

In D46356#1099581, @evandro wrote:

In D46356#1089483, @courbet wrote:

@evandro Do you have benchmarks to evaluate the impact of this change ?

Yes. They're not done yet. Will let you know when I get the results.

Any news ?

Sorry, they slipped my mind. I see a minor improvement overall, especially in FP workloads, with a few minor regressions.

Thank you.

OK, thanks. I'll submit this part then.

[courbet]

I accidentally closed this when submitting r332713.

[courbet]

rebase after r332713

[RKSimon]

What is still outstanding on this? Have regression tests been done on all the AArch64 cpus @javed.absar @evandro ?

It'd be very useful to get this error reporting added to trunk ASAP as people are taking an interest in improving scheduling right now.

[courbet]

In D46356#1120096, @RKSimon wrote:

What is still outstanding on this?

A review of ExynosM1 /ThunderX2, as I'm not familiar with these CPUs.

[RKSimon]

@joel_k_jones Can you comment on these aarch64 model changes please?

[evandro]

In D46356#1120407, @courbet wrote:

A review of ExynosM1 /ThunderX2, as I'm not familiar with these CPUs.

I'm sorry, I completely forgot about M1.

lib/Target/AArch64/AArch64SchedExynosM1.td

329 ↗

(On Diff #147494)

[1, 1]

343 ↗

(On Diff #147494)

[2, 2, 2]

349 ↗

(On Diff #147494)

[2, 1, 1, 1]

356 ↗

(On Diff #147494)

[2, 1, 1, 1, 1]

363 ↗

(On Diff #147494)

[1, 1, 1, 1, 1]

371 ↗

(On Diff #147494)

[1, 1, 1, 1, 1, 1]

377 ↗

(On Diff #147494)

[2, 1, 2, 1]

392 ↗

(On Diff #147494)

[7, 1, 1, 1, 1]

401 ↗

(On Diff #147494)

[1, 7, 1, 7, 1, 1, 1]

412 ↗

(On Diff #147494)

[1, 7, 1, 7, 1, 1, 1, 1, 1]

419 ↗

(On Diff #147494)

[1, 7, 1, 7, 1]

432 ↗

(On Diff #147494)

[1, 7, 1, 7, 1, 1, 1, 1, 1, 1, 1]

[courbet]

Update ExynosM1.

lib/Target/AArch64/AArch64SchedExynosM1.td

343 ↗

(On Diff #147494)

I set NumMicroOps = 6 to be consistent. Is that OK ?

349 ↗

(On Diff #147494)

NumMicroOps = 5

356 ↗

(On Diff #147494)

NumMicroOps = 6

377 ↗

(On Diff #147494)

NumMicroOps = 6

392 ↗

(On Diff #147494)

NumMicroOps = 11

401 ↗

(On Diff #147494)

ditto

412 ↗

(On Diff #147494)

ditto

419 ↗

(On Diff #147494)

ditto

432 ↗

(On Diff #147494)

ditto

[courbet]

• fix btver2

[courbet]

More X86 typos crept in while this was pending; I fixed some harmless ones in r334164, but fixing the ones in BtVer2 would actually introduce semantic differences, so I've added it back here.

[RKSimon]

lib/Target/X86/X86ScheduleBtVer2.td

371 ↗

(On Diff #150267)

These are 2 uops not 4, btver2 splits the issue and execution resource.

[courbet]

lib/Target/X86/X86ScheduleBtVer2.td

371 ↗

(On Diff #150267)

OK cool, thanks. So this is an NFC.

[RKSimon]

lib/Target/X86/X86ScheduleBtVer2.td

371 ↗

(On Diff #150267)

No, you need to set it back to 2

[courbet]

Rebase after r334178.

[courbet]

lib/Target/X86/X86ScheduleBtVer2.td

371 ↗

(On Diff #150267)

Sorry, yes, that's what I meant; I just did not re-export to phabricator. I submitted r334178 with 2 here.

[RKSimon]

@evandro Does AArch64SchedExynosM1.td and AArch64SchedThunderX2T99.td look correct now please?

[evandro]

In D46356#1124577, @courbet wrote:

Update ExynosM1.

No, the number of uops was correct. Don't ask how, but some uops can be dispatched to different units at the same time. Please, restore the original number, unless it creates any error. Then, we can talk.

[evandro]

In D46356#1126217, @RKSimon wrote:

@evandro Does AArch64SchedExynosM1.td and AArch64SchedThunderX2T99.td look correct now please?

Except for my remark above on M1, the changes to X2T99 seem reasonable to me. But I am not familiar with this Cavium processor. It'd be better if someone from Cavium would chime in, but their silence, after so long, might be taken as consent.

[courbet]

revert NumMicroOps changes to ExynosM1.

[courbet]

Rebase after submitting ExynosM1 changes as r334391.

[courbet]

I'll submit the cavium changes in a couple of days unless there are reservations.
@joel_k_jones any comments ?

[evandro]

Thank you, @courbet.

[RKSimon]

LGTM - thank @courbet

[courbet]

Thanks all for the reviews.

llvm/trunk/include/llvm/Target/TargetSchedule.td

 // If a target already has itinerary classes, SchedWriteResources can
 // be used instead to define subtarget specific SchedWrites and map
 // them to processor resources in one place. Then ItinRW can map
 // itinerary classes to the subtarget's SchedWrites.
 //
 // ProcResources indicates the set of resources consumed by the write.
 // 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,
-// regardless of latency, which models a fully pipelined processing
+// ProcResource item at the same position in its list. ResourceCycles
+// can be `[]`: in that case, all resources are consumed for a single
+// cycle, regardless of latency, which models a fully pipelined processing
 // unit. A value of 0 for ResourceCycles means that the resource must
 // be available but is not consumed, which is only relevant for
 // unbuffered resources.
 //
 // By default, each SchedWrite takes one micro-op, which is counted
 // against the processor's IssueWidth limit. If an instruction can
 // write multiple registers with a single micro-op, the subtarget
 // should define one of the writes to be zero micro-ops. If a

llvm/trunk/lib/Target/AArch64/AArch64SchedThunderX2T99.td

 
 
 // ALU, basic
 // Conditional compare
 // Conditional select
 // Address generation
 def : WriteRes<WriteI,       [THX2T99I012]> {
   let Latency = 1;
-  let ResourceCycles = [1, 3];
+  let ResourceCycles = [1];
   let NumMicroOps = 2;
 }
 
 def : InstRW<[WriteI],
             (instregex "ADD?(W|X)r(i|r|s|x)",   "ADDS?(W|X)r(i|r|s|x)(64)?",
                        "AND?(W|X)r(i|r|s|x)",   "ANDS?(W|X)r(i|r|s|x)",
                        "ADC(W|X)r",
                        "BIC?(W|X)r(i|r|s|x)",   "BICS?(W|X)r(i|r|s|x)",
                        "EON?(W|X)r(i|r|s|x)",   "ORN?(W|X)r(i|r|s|x)",
                        "ORR?(W|X)r(i|r|s|x)",   "SUB?(W|X)r(i|r|s|x)",
                        "SUBS?(W|X)r(i|r|s|x)",  "SBC(W|X)r",
                        "SBCS(W|X)r",            "CCMN(W|X)(i|r)",
                        "CCMP(W|X)(i|r)",        "CSEL(W|X)r",
                        "CSINC(W|X)r",           "CSINV(W|X)r",
                        "CSNEG(W|X)r")>;
 
 def : InstRW<[WriteI], (instrs COPY)>;
 
 // ALU, extend and/or shift
 def : WriteRes<WriteISReg,   [THX2T99I012]> {
   let Latency = 2;
-  let ResourceCycles = [2, 3];
+  let ResourceCycles = [2];
   let NumMicroOps = 2;
 }
 
 def : InstRW<[WriteISReg],
             (instregex "ADD?(W|X)r(i|r|s|x)",   "ADDS?(W|X)r(i|r|s|x)(64)?",
                        "AND?(W|X)r(i|r|s|x)",   "ANDS?(W|X)r(i|r|s|x)",
                        "ADC(W|X)r",
                        "BIC?(W|X)r(i|r|s|x)",   "BICS?(W|X)r(i|r|s|x)",
                        "EON?(W|X)r(i|r|s|x)",   "ORN?(W|X)r(i|r|s|x)",
                        "ORR?(W|X)r(i|r|s|x)",   "SUB?(W|X)r(i|r|s|x)",
                        "SUBS?(W|X)r(i|r|s|x)",  "SBC(W|X)r",
                        "SBCS(W|X)r",            "CCMN(W|X)(i|r)",
                        "CCMP(W|X)(i|r)",        "CSEL(W|X)r",
                        "CSINC(W|X)r",           "CSINV(W|X)r",
                        "CSNEG(W|X)r")>;
 
 def : WriteRes<WriteIEReg,   [THX2T99I012]> {
   let Latency = 1;
-  let ResourceCycles = [1, 3];
+  let ResourceCycles = [1];
   let NumMicroOps = 2;
 }
 
 def : InstRW<[WriteIEReg],
             (instregex "ADD?(W|X)r(i|r|s|x)",   "ADDS?(W|X)r(i|r|s|x)(64)?",
                        "AND?(W|X)r(i|r|s|x)",   "ANDS?(W|X)r(i|r|s|x)",
                        "ADC(W|X)r",
                        "BIC?(W|X)r(i|r|s|x)",   "BICS?(W|X)r(i|r|s|x)",

 //---
 // 3.4 Divide and Multiply Instructions
 //---
 
 // Divide, W-form
 // Latency range of 13-23/13-39.
 def : WriteRes<WriteID32,    [THX2T99I1]> {
   let Latency = 39;
-  let ResourceCycles = [13, 39];
+  let ResourceCycles = [39];
   let NumMicroOps = 4;
 }
 
 // Divide, X-form
 def : WriteRes<WriteID64,    [THX2T99I1]> {
   let Latency = 23;
-  let ResourceCycles = [13, 23];
+  let ResourceCycles = [23];
   let NumMicroOps = 4;
 }
 
 // Multiply accumulate, W-form
 def : WriteRes<WriteIM32,    [THX2T99I012]> {
   let Latency = 5;
   let NumMicroOps = 3;
 }

 // ASIMD shift by immed, complex
 // ASIMD shift by register, basic, D-form
 // ASIMD shift by register, basic, Q-form
 // ASIMD shift by register, complex, D-form
 // ASIMD shift by register, complex, Q-form
 def : WriteRes<WriteV, [THX2T99F01]> {
   let Latency = 7;
   let NumMicroOps = 4;
-  let ResourceCycles = [4, 23];
+  let ResourceCycles = [4];
 }
 
 // ASIMD arith, reduce, 4H/4S
 // ASIMD arith, reduce, 8B/8H
 // ASIMD arith, reduce, 16B
 
 // ASIMD logical (MVN (alias for NOT), ORN, ORR)
 def : InstRW<[THX2T99Write_5Cyc_F01],

llvm/trunk/utils/TableGen/SubtargetEmitter.cpp

Record *SubtargetEmitter::FindReadAdvance(const CodeGenSchedRW &SchedRead,
   return ResDef;
 }
 
 // Expand an explicit list of processor resources into a full list of implied
 // resource groups and super resources that cover them.
 void SubtargetEmitter::ExpandProcResources(RecVec &PRVec,
                                            std::vector<int64_t> &Cycles,
                                            const CodeGenProcModel &PM) {
-  // Default to 1 resource cycle.
-  Cycles.resize(PRVec.size(), 1);
+  assert(PRVec.size() == Cycles.size() && "failed precondition");
   for (unsigned i = 0, e = PRVec.size(); i != e; ++i) {
     Record *PRDef = PRVec[i];
     RecVec SubResources;
     if (PRDef->isSubClassOf("ProcResGroup"))
       SubResources = PRDef->getValueAsListOfDefs("Resources");
     else {
       SubResources.push_back(PRDef);
       PRDef = SchedModels.findProcResUnits(PRDef, PM, PRDef->getLoc());
for (unsigned W : Writes) {
         SCDesc.BeginGroup |= WriteRes->getValueAsBit("SingleIssue");
         SCDesc.EndGroup |= WriteRes->getValueAsBit("SingleIssue");
 
         // Create an entry for each ProcResource listed in WriteRes.
         RecVec PRVec = WriteRes->getValueAsListOfDefs("ProcResources");
         std::vector<int64_t> Cycles =
           WriteRes->getValueAsListOfInts("ResourceCycles");
 
+        if (Cycles.empty()) {
+          // If ResourceCycles is not provided, default to one cycle per
+          // resource.
+          Cycles.resize(PRVec.size(), 1);
+        } else if (Cycles.size() != PRVec.size()) {
+          // If ResourceCycles is provided, check consistency.
+          PrintFatalError(
+              WriteRes->getLoc(),
+              Twine("Inconsistent resource cycles: !size(ResourceCycles) != "
+                    "!size(ProcResources): ")
+                  .concat(Twine(PRVec.size()))
+                  .concat(" vs ")
+                  .concat(Twine(Cycles.size())));
+        }
+
         ExpandProcResources(PRVec, Cycles, ProcModel);
 
         for (unsigned PRIdx = 0, PREnd = PRVec.size();
              PRIdx != PREnd; ++PRIdx) {
           MCWriteProcResEntry WPREntry;
           WPREntry.ProcResourceIdx = ProcModel.getProcResourceIdx(PRVec[PRIdx]);
           assert(WPREntry.ProcResourceIdx && "Bad ProcResourceIdx");
           WPREntry.Cycles = Cycles[PRIdx];