[PATCH] D117405: [AArch64] CodeGen for Armv8.8/9.3 MOPS

[Son Tuan Vu via Phabricator via llvm-commits]

tyb0807 marked 8 inline comments as done.
tyb0807 added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:836
+  // inside a bundle to prevent other passes to moving things in between.
+  MIBundleBuilder Bundler(MBB, MBBI);
+  auto &MF = *MBB.getParent();
----------------
dmgreen wrote:
> It is probably better to expand the instruction later than to create a bundle.
Please see https://reviews.llvm.org/D117763, the instruction is now expanded as late as possible in the pipeline (i.e. during code emission)


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:940
+  if (Subtarget->hasMOPS()) {
+    // If we have MOPS, always use them
+    MaxStoresPerMemsetOptSize = 0;
----------------
dmgreen wrote:
> Are you sure we should _always_ be using them? I would expect a `ldr+str` to be better than a `mov #4; cpyfp; cpyfm; cpyfe`, for example.
Please see https://reviews.llvm.org/D117764, we are now conservative on when to use MOPS instructions instead of `ldr+str`


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11875
+    Info.opc = ISD::INTRINSIC_W_CHAIN;
+    Info.memVT = MVT::getVT(Val->getType());
+    Info.ptrVal = Dst;
----------------
dmgreen wrote:
> Can this safely set memVT to the size of a single element? That would need to be multiplied by the size of the memory being set, and if this is being used for aliasing info will be too small.
Please see https://reviews.llvm.org/D117764, the size is set to unknown.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:8350
+  let mayLoad = 1 in {
+    def MOPSMemoryCopy : Pseudo<(outs GPR64common:$Rd_wb, GPR64common:$Rs_wb, GPR64:$Rn_wb),
+                                (ins GPR64common:$Rd, GPR64common:$Rs, GPR64:$Rn),
----------------
dmgreen wrote:
> These need to be marked as 96bit pseudos. They should also clobber NZCV (as should the real instructions above, but that doesn't look like it was ever changed after D116157)
Changes to make real instructions NZCV clobbering here https://reviews.llvm.org/D117757

Changes to make pseudos NZCV clobbering (and specifying their size) here https://reviews.llvm.org/D117764


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-mops-mte.ll:3
+
+; RUN: llc %s -o - -mtriple=aarch64-arm-none-eabi -O2 -mattr=+mops,+mte  | FileCheck %s --check-prefix=SDAG
+; RUN: llc %s -o - -mtriple=aarch64-arm-none-eabi -O0 -global-isel=1 -global-isel-abort=1 -mattr=+mops,+mte  | FileCheck %s --check-prefix=GISel
----------------
dmgreen wrote:
> You likely don't need the -O2 and -O0 for llc run commands.
Agreed, we can do this without. However, this will change the generated code (thus the  strings that these tests are looking for), hence I'm leaving those options. Please lmk if this is _really_ unwanted and I'll try to fix these tests 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117405