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

[Simon Moll via Phabricator via llvm-commits]

simoll marked an inline comment as done.
simoll 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,
----------------
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.


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


================
Comment at: llvm/lib/Target/VE/VVPInstrPatternsVec.td:35
+
+  // TODO immediate variants.
+  // TODO Fold vvp_select into passthru.
----------------
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


================
Comment at: llvm/lib/Target/VE/VVPNodes.inc:1
+//===-- VVPNodes.def - Lists & properties of VE Vector Predication Nodes --===//
+//
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91802