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

[Kazushi Marukawa via Phabricator via llvm-commits]

kaz7 added inline comments.


================
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,
----------------
simoll wrote:
> kaz7 wrote:
> > 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.
> Using `1` for now.. i'll factor out stuff like this into a custom DAG class that wraps common `dag.getNode/getConstant/`, etc. cases later.
Using 1 here is fine to me.


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


================
Comment at: llvm/lib/Target/VE/VVPInstrPatternsVec.td:35
+
+  // TODO immediate variants.
+  // TODO Fold vvp_select into passthru.
----------------
simoll wrote:
> kaz7 wrote:
> > 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.  ;-)
> > 
> > 
> Can we do 2), please? :) It makes sense on so many accounts
Thant's fine to me.  It's better to remove existing pattern using "ivl" for the consistency.  And it's much better to add some comments that we will use fold immediate in peephole optimizaiton for vector instructions having immediate operands.


================
Comment at: llvm/lib/Target/VE/VVPNodes.inc:1
+//===-- VVPNodes.def - Lists & properties of VE Vector Predication Nodes --===//
+//
----------------
simoll wrote:
> kaz7 wrote:
> > 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
> > ```
> I will rename this to `VVPNodes.def` and simplify it to use only a `ADD_VVP_OP(VVP, ISD)` macro.
> 
> Not sure about the macro-usage examples. I am already doing that, eg see the definition of the `TARGET_NODE_NAME` macro and include in VEISelLowering with this patch.
> 
> About caller/includer-side-only def/undef: the macros are undefined in the `VVPNodes.inc/def` file because when we include it, we only want to define a few of the macros that are actually used in the file to query specific properties (there will be more macros in the future). By undefining all macros in the def-file itself the includer is not poluted with macros they do not care about.
Wait.  I was thinking examples containing only binary operators.  So, I asked you to use single `ADD_VVP_OP` macro to simplify them.

However, I've understood what you want to write here is not only binary but also unary operators like below:
```
ADD_VVP_OP(VVP_ADD)       REGISTER_BINARY_VVP_OP(VVP_ADD, ADD)
ADD_VVP_OP(VVP_NOT)       REGISTER_UNARY_VVP_OP(VVP_NOT, NOT)
```

We cannot write `ADD_VVP_OP(VVP_ADD, ADD)` in this case since we want to separate binary and unary operators.  So, my suggestion doesn't work in this case.  Please leave macros as original your codes.

I still think we can simplify these macros even if we need to write binary, unary, and otehr operators, but I have no concrete examples at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91802