[PATCH] D91802: [VE] VE Vector Predicated SDNode, vector add isel and tests

[Kazushi Marukawa via Phabricator via llvm-commits]

kaz7 added a comment.

Small checks like 80 columns and indentation.  And one question about how you plan to implement test cases for fold immediates.



================
Comment at: llvm/lib/Target/VE/VEISelLowering.cpp:259
     setOperationAction(ISD::BUILD_VECTOR, LegalVecVT, Custom);
+    // Translate all vector instructions with legal element types to VVP_* nodes
+    // TODO we will custom-widen into VVP_* nodes in the future. While we are
----------------
Indentation.


================
Comment at: llvm/lib/Target/VE/VEInstrInfo.td:249
+def nonzero        : PatLeaf<(imm), [{ return N->getSExtValue() !=0 ; }]>;
+
 
----------------
Additional empty line.  It's OK if you intended it.


================
Comment at: llvm/lib/Target/VE/VEInstrInfo.td:2230
+class IsVLVT<int OpIdx> : SDTCisVT<OpIdx,i32>;
+def vec_broadcast   : SDNode<"VEISD::VEC_BROADCAST", SDTypeProfile<1, 2,  [SDTCisVec<0>, IsVLVT<2>]>>;
+
----------------
Over 80 columns.  Following several lines also has over 80 columns.


================
Comment at: llvm/lib/Target/VE/VEInstrInfo.td:248
                                         == 0; }]>;
+def True        : PatLeaf<(imm), [{ return N->getSExtValue() !=0 ; }]>;
+
----------------
kaz7 wrote:
> simoll wrote:
> > kaz7 wrote:
> > > Is there any definition about capitalization about PatLeaf?  I used lozero, fplozero, and etc.  Whole lower case.  If capitalization is common, I'd like to modify exisiting PatLeaves.
> > I was shying away from calling it `true` as it seemed too close to C++.. capitalizing it at least makes it stand out as something non-builtin.
> > I am not aware of capitalization rules for TableGen. Being consistent on our end is the least we can do however. Suggestions for a lower-case name?
> I have no real opinion, but suggest for a lower-case name for the consistency.
Indentation before `:`


================
Comment at: llvm/lib/Target/VE/VVPInstrInfo.td:1
+//===---------- VVPInstrInfo.td - VVP_* SDNode patterns----------------===//
+//
----------------
80 columns.


================
Comment at: llvm/lib/Target/VE/VVPInstrInfo.td:10
+// This file defines the VE Vector Predicated SDNodes (VVP SDNodes).
+// VVP SDNodes are an intermediate isel layer between the vector SDNodes emitted by LLVM 
+// and the actual VE vector instructions. For example:
----------------
Over 80 columns.


================
Comment at: llvm/lib/Target/VE/VVPInstrInfo.td:25
+def SDTIntBinOpVVP : SDTypeProfile<1, 4, [     // vp_add, vp_and, etc.
+  SDTCisSameAs<0, 1>, SDTCisSameAs<0, 2>, SDTCisInt<0>, SDTCisSameNumEltsAs<0, 3>, IsVLVT<4>
+]>;
----------------
Over 80 columns.


================
Comment at: llvm/lib/Target/VE/VVPInstrPatternsVec.td:20
+
+multiclass VectorBinaryArith<SDPatternOperator OpNode, ValueType ScalarVT, ValueType DataVT, ValueType MaskVT, string OpBaseName, SDPatternOperator ImmOp, SDNodeXForm ImmCast> {
+  // No mask.
----------------
Over 80 columns.  Following lines also has over 80 columns.


================
Comment at: llvm/test/CodeGen/VE/Vector/vec_add.ll:76
+}
+
+; Function Attrs: nounwind
----------------
Removing regression test cases like `add_iv_v256i64()` means that you need to implement `iv` test cases for all vector instructions when you implement a FoldImmediate for all vector instructions.  Just want to make sure that you intend it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91802