[PATCH] D67485: AArch64: use ldp/stp for atomic & volatile 128-bit where appropriate.

Mon Sep 23 07:46:53 PDT 2019

kristof.beyls added a comment.

In D67485#1676748 <https://reviews.llvm.org/D67485#1676748>, @t.p.northover wrote:

> > Enabling the corresponding subtarget feature on cyclone doesn't seem safe to me. If we ever implement -mtune, then a command line like clang -march=armv8a -mtune=cyclone should mean “generated correct code for the v8.0 architecture, but optimize for cyclone". Adding this feature to cyclone as is would probably result in the above command line producing code that isn’t architecturally correct for v8.0.
>
> I don't think Clang really does anything with -mtune yet. The most it could do based on the way CPUs are implemented in LLVM now would be something like applying the scheduling model. Almost all of the features in the list are going to break older CPUs.

I'm afraid I mentioned to Alexandros that I wondered how this would interact with a potential future enabling for an -mtune feature, leading to the above question.
But you're right, if we do end up implementing support for -mtune, we'd need to categorize subtarget features into either architectural or tuning features, and only enable the tuning features for a subtarget when -mtune is used. Clearly, FeatureAtomicLDPSTP is an architectural feature. So this part of the patch is just fine I think.
Sorry for the noise.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67485/new/

https://reviews.llvm.org/D67485