[PATCH] D99355: Implementation of intrinsic and SDNode definitions for VP load, store, gather, scatter.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

Just some minor comments. I think Craig's suggestion of mem subclasses sounds like it's worth investigating.



================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:1367
     // with either an intrinsic or a target opcode.
-    return N->getOpcode() == ISD::LOAD                ||
-           N->getOpcode() == ISD::STORE               ||
-           N->getOpcode() == ISD::PREFETCH            ||
-           N->getOpcode() == ISD::ATOMIC_CMP_SWAP     ||
+    return N->getOpcode() == ISD::LOAD || N->getOpcode() == ISD::STORE ||
+           N->getOpcode() == ISD::PREFETCH ||
----------------
craig.topper wrote:
> Can we override clang-format here? The previous code had a more consistent structure.
Alternatively, I suppose, it could be a switch statement (aside from the last two)? Presumably that'd be optimized well enough by compilers?


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:409
+  Value *getMemoryPointerParam() const;
+  static Optional<int> GetMemoryPointerParamPos(Intrinsic::ID);
+
----------------
Inconsistent documentation style, e.g. `return the` and `return The`, and use of commas vs forward slashes.

Also, is clang-tidy correct here? No uppercase letter on static methods?


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1346
+                               llvm_i32_ty],
+                             [ NoCapture<ArgIndex<1>>, IntrNoSync, IntrArgMemOnly, IntrWillReturn ]>;
+
----------------
How some the loads are `IntrReadMem` but the stores aren't `IntrWriteMem`?


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:91
 
+// Map this VP intrinsic to its cannonical functional intrinsic.
+#ifndef HANDLE_VP_TO_INTRIN
----------------
typo: `cannonical` -> `canonical`


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:305
+Value *VPIntrinsic::getMemoryPointerParam() const {
+  auto PtrParamOpt = GetMemoryPointerParamPos(getIntrinsicID());
+  if (!PtrParamOpt.hasValue())
----------------
Purely a matter of style but this is slightly shorter and (I think) a little clearer:
```
if (auto PtrParamOpt = GetMemoryPointerParamPos(getIntrinsicID()))
  return getArgOperand(PtrParamOpt.getValue());
return nullptr;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99355