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

[Kazushi Marukawa via Phabricator via llvm-commits]

kaz7 added a comment.

I add some comments inlined.



================
Comment at: llvm/lib/Target/VE/VEISelLowering.cpp:1701
+  MVT MaskVT = MVT::v256i1;
+  SDValue ConstTrue = DAG.getConstant(-1, DL, MVT::i32);
+  SDValue Mask = DAG.getNode(VEISD::VEC_BROADCAST, DL, MaskVT,
----------------
If you want to implement something generic method, you can check `getBooleanContents(true, false)` to check the immediate value of vector true.  For the case of VE, it is 1 not -1.


================
Comment at: llvm/lib/Target/VE/VEInstrInfo.td:248
                                         == 0; }]>;
+def True        : PatLeaf<(imm), [{ return N->getSExtValue() !=0 ; }]>;
+
----------------
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.


================
Comment at: llvm/lib/Target/VE/VVPInstrPatternsVec.td:35
+
+  // TODO immediate variants.
+  // TODO Fold vvp_select into passthru.
----------------
I guess missing immediate variant is only "ivml".  How about impelementing it?

On the other hand, we have another choice which is not to implement immediate variants anyway.  Then, we can ask fold immediate to do so (Not implemented yet for vector instructions, though).  We have too many immediate variants.

I think we can choose several ways here, 1) implement whole immediate variants, 2) not implement immediate variants anyway and use fold immediate, 3) implement some important immeidate variants only and use fold immeidate.  Do you have any opinion here?  I think 2 is fine if you would like to implement more VVP and don't have time to work in immediate variants.

In addition, fold immediate for vector instructions is not implemented yet, but soon.  ;-)




================
Comment at: llvm/lib/Target/VE/VVPNodes.inc:1
+//===-- VVPNodes.def - Lists & properties of VE Vector Predication Nodes --===//
+//
----------------
I have several suggestions.
  1. How about using .def as a file extension, similar to `include/llvm/BinaryFormat/ELFRelocs/VE.def` and otehrs.  I feel `.inc` is used for tablegen generated files.
  2. How about using the method similar to others like '#define' and '#undef' at caller side.
  3. You can write only `VVP_MAP(VVP_ADD, ADD)` in this file.  Then, caller side can define `VVP_MAP` in several ways.  e.g.,:
```
#define VVP_MAP(X, Y) TARGET_NODE_CASE(X)
#include ...
#undef VVP_MAP
```
or
```
#define VVP_MAP(X, Y) \
  case VEISD::X: \
  case ISD::Y: \
    return VEISD::X;
#include ...
#undef VVP_MAP
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91802