[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:34
 #include "llvm/Analysis/VectorUtils.h"
+#include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/CodeGen/Analysis.h"
----------------
Clang format should put these in order


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:940
 
-  // In case of strict alignment, avoid an excessive number of byte wide stores.
+  // FIXME: right now we are being conservative on when it is best to use
+  // MOPS instructions instead of ldr/str sequence(s), i.e. we are emitting
----------------
I'm not sure this is worth adding - if it turns out that the existing costs are fine then it will be left as a FIXME for all time without adding much useful information.


================
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);
----------------
tyb0807 wrote:
> dmgreen wrote:
> > 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.
> I'm not sure. For `Intrinsic::memcpy` and `Intrinsic::memmove`, from `Intrinsics.td`, it seems that the type for `SrcOrValue` is `llvm_anyptr_ty`. Am I missing something here?
The condition doesn't need getSimpleVT().
I think this should only happen with IsSet, so it's probably worth sinking into the if below. The it might as well be if `if (SrcOrValue.getValueType() != MVT::i64)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117764