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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:940
 
-  // In case of strict alignment, avoid an excessive number of byte wide stores.
-  MaxStoresPerMemsetOptSize = 8;
-  MaxStoresPerMemset = Subtarget->requiresStrictAlign()
-                       ? MaxStoresPerMemsetOptSize : 32;
+  if (Subtarget->hasMOPS()) {
+    // FIXME: right now we are being conservative on when it is best to use
----------------
I think that if the values in both sides of the if are currently the same - it's probably worth removing the if. We can always re-add it again if/when we alter these in the future.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4111
+    // MOPS_MEMSET_TAGGING has 3 results (DstWb, SizeWb, Chain) whereas the
+    // intrinsic has 2. So hide SizeWb it using MERGE_VALUES. Otherwise
+    // LowerOperationWrapper will complain that the number of results has
----------------
-> "So hide SizeWb using MERGE_VALUES."


================
Comment at: llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp:26
+
+  // Get the constant size of the copy/set. We don't use it.
+  uint64_t ConstSize = 0;
----------------
What does "We don't use it" mean here?


================
Comment at: llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp:63
+  // Extend i8 value to i64 if required
+  if (SrcOrValue.getValueType().getSimpleVT() == MVT::i8) {
+    SrcOrValue = DAG.getNode(ISD::ANY_EXTEND, DL, MVT::i64, SrcOrValue);
----------------
Maybe use != MVT::i8, or add an assert. I guess it will always be an i8 or a i64? Will this only be for Set?

In llvm, singled statements don't need brackets around them and can be dropped.


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-mops-consecutive.ll:10
+
+define hidden void @consecutive() local_unnamed_addr {
+; CHECK-MOPS-LABEL: consecutive:
----------------
You can usually remove "hidden" "dso_local" and "local_unnamed_addr", to clean up the test a little. "Function Attrs: " too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117764