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

Dave Green via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 17 01:19:45 PST 2022

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list