Page MenuHomePhabricator

[AArch64] Refactor the Exynos scheduling predicates
ClosedPublic

Authored by evandro on Wed, Dec 5, 4:11 PM.

Details

Summary

Refactor the scheduling predicates based on MCInstPredicate. In this case, for the Exynos processors.

Diff Detail

Repository
rL LLVM

Event Timeline

evandro created this revision.Wed, Dec 5, 4:11 PM

Hi Evandro,

Thanks for the patch.

Could you please update the diff?
I tried to apply your patch, but there were failures.
AArch64InstrInfo::isExynosShiftFast() is referenced by your patch, but it doesn't appear to exist upstream. In another case, the conflict was caused by a diff which appears to have been committed already.

Thanks,
Andrea

llvm/lib/Target/AArch64/AArch64.td
130–133 ↗(On Diff #176891)

This change was already committed at r348398.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
708 ↗(On Diff #176891)

There is no isExynosShiftFast().

llvm/lib/Target/AArch64/AArch64InstrInfo.h
253–255 ↗(On Diff #176891)

Not sure where isExynosShiftFast declaration comes from. It is not available at HEAD revision. So, this patch fails to apply.

evandro updated this revision to Diff 176987.Thu, Dec 6, 8:43 AM
evandro retitled this revision from [AArch64] Refactor the scheduling predicates (NFC) to [AArch64] Refactor the scheduling predicates.

Rebase the patch.

Rebase the patch.

Unfortunally, I am still seeing several tablegen errors.

Tablegen complains about missing definitions. It looks like most (if not all) of those missing definitions will be added by D55375.
I reported only a few missing symbols. Is this patch meant to be dependent on D55375? If not, then this patch should be fixed. If it is going to be complicated, then I don't mind if you merge the two patches into a single diff. As long as I can build it, and check that the generated code is still semantically equivalent to what there was before, then I am happy.

Although this patch is supposed to be an NFC, I'd like to see at least one llvm-mca test. Ideally, a test that shows how we can now analyze instructions that llvm-mca was previously unable to analyze.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
940–941 ↗(On Diff #176987)

For the record: this method never existed in the original code. Not sure how it ended up in this diff.

llvm/lib/Target/AArch64/AArch64SchedPredExynos.td
25 ↗(On Diff #176987)

IsArithExt32Op is not defined.

27 ↗(On Diff #176987)

CheckExtBy0 is not defined.

35 ↗(On Diff #176987)

IsArithExt64Op is not defined.

48 ↗(On Diff #176987)

These two definitions are missing. I saw you added those in D55375.

evandro updated this revision to Diff 177009.Thu, Dec 6, 10:45 AM
evandro retitled this revision from [AArch64] Refactor the scheduling predicates to [AArch64] Refactor the Exynos scheduling predicates.

Sorry, I was juggling too many patches out of order.

I'd appreciate your reviewing the new predicates and their application while I work on test cases.

Thank you.

Sorry, I was juggling too many patches out of order.

I'd appreciate your reviewing the new predicates and their application while I work on test cases.

Thank you.

No problem! I am happy to review your patches :-).

evandro marked 8 inline comments as done.Thu, Dec 6, 3:52 PM

I went through the various changes, and I didn’t find anything that looks obviously wrong. The new predicates look fine to me.

For the tests, I recommend you to use flag -instruction-table. As an example, see the RUN line in test/tools/llvm-mca/X86/BtVer2/resources-avx1.s.

Flag -instruction-tables enables printing the theoretical resource distribution. In my experience, that’s the best way to check the resource consumption of every instruction.
That flag, plus the implicit instruction-info view, is the best combination for tests where you want to verify latency/throughput and resource usage.

It is particularly useful in the presence of instructions that require resolving write variants; you can basically check all the interesting variants.

As soon as tests are added, and you are happy with the latency/throughput/resource usage of instructions, I will be happy to accept this patch.

-Andrea

evandro updated this revision to Diff 177531.Mon, Dec 10, 8:47 AM
andreadb accepted this revision.Mon, Dec 10, 9:10 AM

LGTM.

Thanks Evandro.

This revision is now accepted and ready to land.Mon, Dec 10, 9:10 AM
This revision was automatically updated to reflect the committed changes.