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

[Dave Green via Phabricator via cfe-commits]

dmgreen added a comment.

This looks like a large patch. It may be better split up into a base/isel part, a clang part and a global isel part (which may require different reviewers).



================
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();
----------------
It is probably better to expand the instruction later than to create a bundle.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:940
+  if (Subtarget->hasMOPS()) {
+    // If we have MOPS, always use them
+    MaxStoresPerMemsetOptSize = 0;
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11875
+    Info.opc = ISD::INTRINSIC_W_CHAIN;
+    Info.memVT = MVT::getVT(Val->getType());
+    Info.ptrVal = Dst;
----------------
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.


================
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),
----------------
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)


================
Comment at: llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp:28
+  uint64_t ConstSize = 0;
+  if (auto *C = dyn_cast<ConstantSDNode>(Size)) {
+    ConstSize = cast<ConstantSDNode>(Size)->getZExtValue();
----------------
LLVM doesn't need brackets around a single statement.


================
Comment at: llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp:48
+    }
+    llvm_unreachable_internal("Unhandled MOPS ISD Opcode");
+    return AArch64::INSTRUCTION_LIST_END;
----------------
llvm_unreachable is far more common than llvm_unreachable_internal. It can be moved into the default case too, then it doesn't need a return after it.


================
Comment at: llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp:53-54
+  MachineMemOperand::Flags Flags = MachineMemOperand::MOStore;
+  // if (!Temporal)
+  //   Flags |= MachineMemOperand::MONonTemporal;
+  if (isVolatile)
----------------
There is commented out code here.


================
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
----------------
You likely don't need the -O2 and -O0 for llc run commands.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117405