[PATCH] D70176: [Codegen][ARM] Add addressing modes from masked loads and stores

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Nice one. Haven't looked at everything yet, but some first nits inlined.



================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:2313
+  const SDValue &getOffset() const {
+    return getOperand(getOpcode() == ISD::LOAD ? 2 : 3);
+  }
----------------
This should be `ISD::MLOAD`?


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2116
+  /// Indicate that the specified indexed masked load does or does not work with
+  /// the specified type and indicate what to do abort it.
+  ///
----------------
abort -> about?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13550
 
   bool isLoad = true;
+  bool IsMasked = false;
----------------
While you're at it, to make things a bit more consistent: isLoad -> IsLoad ?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13810
 
   bool isLoad = true;
+  bool IsMasked = false;
----------------
Same here?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13814
   EVT VT;
-  if (LoadSDNode *LD  = dyn_cast<LoadSDNode>(N)) {
+  if (LoadSDNode *LD = dyn_cast<LoadSDNode>(N)) {
     if (LD->isIndexed())
----------------
Is this big if-statement exactly the same as in `DAGCombiner::CombineToPreIndexedLoadStore`, except the ISD nodes? Can this be a helper function?


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

https://reviews.llvm.org/D70176