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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
