This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add pipeline model for Exynos M3
ClosedPublic

Authored by evandro on Jan 22 2018, 11:26 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

evandro created this revision.Jan 22 2018, 11:26 AM

Ping! ๐Ÿ””

MatzeB accepted this revision.Jan 29 2018, 9:40 AM

I can't say much about the scheduling model itself as I'm not familiar with Exynos.

LGTM if you introduce a subtarget feature for the isCheapAsAMove behavior (see below).

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
678โ€“679 โ†—(On Diff #130931)

Looks like it is time to make a subtarget feature out of this.

llvm/test/CodeGen/AArch64/misched-fusion-aes.ll
1โ€“9 โ†—(On Diff #130931)

More candidates here where at some point we better start testing -mattr=+fuse-aes / -mattr=-fuse-aes and have a seperate test to check target feature lists of CPUs...

(but as I noted in my other review that is work for another time/commit).

This revision is now accepted and ready to land.Jan 29 2018, 9:40 AM

Actually can you rename the scheduling model to AArch64SchedExynosM3.td? (Feel free to do the same with the M1 file in a separate commit without review).

evandro added inline comments.Jan 29 2018, 12:10 PM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
678โ€“679 โ†—(On Diff #130931)

I agree in principle, but there already is FeatureCustomCheapAsMoveHandling, so I'm afraid that it might be a bit more involved and that it perhaps deserves a separate discussion. Basically, these checks customize the custom handling of isAsCheapAsAMove because Exynos is different from the other AArch64 targets.

Actually can you rename the scheduling model to AArch64SchedExynosM3.td? (Feel free to do the same with the M1 file in a separate commit without review).

The filename change for M1 was done in rL323686.

MatzeB added inline comments.Jan 29 2018, 1:23 PM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
678โ€“679 โ†—(On Diff #130931)

On a general level: I really think we should push towards not using getProcFamily() outside AArch64Subtarget.cpp; It's a sign that things aren't modeled cleanly and it'll only get worse with new exception added. And while I wouldn't mind a case like this in isolation, in practice people copy the patterns they see elsewhere and I don't want this to be the copy&paste source for more getProcFamily() calls in the target.

Adding FeatureExynosCheapAsMoveHandling (or a better name if you have one) and using that here shouldn't be that hard. It's unfortunate that it would only kick in when CustomCheapAsMoveHandling is enabled too, but not a dealbreaker or reason not introduce the subtarget feature here.

evandro added inline comments.Jan 29 2018, 1:57 PM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
678โ€“679 โ†—(On Diff #130931)

I had to point it out, but, if it sounds sensible to you, I do not object to it.

This revision was automatically updated to reflect the committed changes.