[PATCH] D42387: [AArch64] Add pipeline model for Exynos M3

[Evandro Menezes via Phabricator via llvm-commits]

evandro added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:678-679
     return MI.isAsCheapAsAMove();
-  if (Subtarget.getProcFamily() == AArch64Subtarget::ExynosM1 &&
+  if ((Subtarget.getProcFamily() == AArch64Subtarget::ExynosM1 ||
+       Subtarget.getProcFamily() == AArch64Subtarget::ExynosM3) &&
       isExynosShiftLeftFast(MI))
----------------
MatzeB wrote:
> evandro wrote:
> > MatzeB wrote:
> > > Looks like it is time to make a subtarget feature out of this.
> > 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.
> 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.
I had to point it out, but, if it sounds sensible to you, I do not object to it.


Repository:
  rL LLVM

https://reviews.llvm.org/D42387