[PATCH] D57504: RFC: Prototype & Roadmap for vector predication in LLVM

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Hi Simon, I went through the code for the first time, and this is a first round of proper nitpicks from my side. Please ignore if you want to focus on the bigger picture at this point in the discussion, but these are just some things I noticed. General nitpick is that you should run clang-format as there are quite a few coding style issues: indentation, indentation of arguments, exceeding 80 columns, placement of `*` and `&` in arguments and return values, etc.  And find some more nitpicks inlined.



================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:489
+
+    /// Select with an integer pivot (op #0) and two vector operands (ops #1
+    /// and #2), returning a vector result.  All vectors have the same length.
----------------
I was unfamiliar with this one... I think I know what it does, and how it is different from VP_SELECT, but for clarity, can you define what `integer pivot` is?


================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:492
+    /// Similar to the vector select, a comparison of the results element index
+    /// with the integer pivot selects hether the corresponding result element
+    /// is taken from op #1 or op #2.
----------------
typo: hether


================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:1134
+  /// Return the mask operand of this VP SDNode.
+  /// Otw, return -1.
+  int GetMaskPosVP(unsigned OpCode);
----------------
just spell out 'otherwise' here, and also below.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:727
 
+  /// Test whether this is an Explicit Vector Length node.
+  bool isVP() const {
----------------
Perhaps outdated comment? Should it be  something along the lines of 'vector predicated node' i.s.o. explicit vector lenght node?


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:1492
            N->getOpcode() == ISD::MSCATTER            ||
+           N->getOpcode() == ISD::VP_LOAD            ||
+           N->getOpcode() == ISD::VP_STORE           ||
----------------
indentation of `||` off by 1?


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:2357
 
+/// This base class is used to represent MLOAD and MSTORE nodes
+class VPLoadStoreSDNode : public MemSDNode {
----------------
`VP_LOAD` and `VP_STORE`?


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:2386
+
+/// This class is used to represent an MLOAD node
+class VPLoadSDNode : public VPLoadStoreSDNode {
----------------
same?


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:2426
+
+  /// Return true if the op does a truncation before store.
+  /// For integers this is the same as doing a TRUNCATE and storing the result.
----------------
`.. does a truncation before store` sounds a bit odd. Since 'truncating store' is a well known term, and that you explain what it is for ints/floats below, I think it suffices to say "Return true if this is truncating store. For intergers ..."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57504