This is an archive of the discontinued LLVM Phabricator instance.

[VE] VE Vector Predicated SDNode, vector add isel and tests
ClosedPublic

Authored by simoll on Nov 19 2020, 9:17 AM.

Details

Summary

VE Vector Predicated (VVP) SDNodes form an intermediate layer between VE vector instructions and the initial SDNodes.

We introduce 'vvp_add' with isel and tests as the first of these VVP nodes. VVP nodes have a mask and explicit vector length operand, which we will make proper use of later.

Diff Detail

Event Timeline

simoll created this revision.Nov 19 2020, 9:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2020, 9:17 AM
simoll requested review of this revision.Nov 19 2020, 9:17 AM
kaz7 added a comment.Nov 20 2020, 11:04 PM

I add some comments inlined.

llvm/lib/Target/VE/VEISelLowering.cpp
1678

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.

llvm/lib/Target/VE/VEInstrInfo.td
248

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.

llvm/lib/Target/VE/VVPInstrPatternsVec.td
36

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. ;-)

llvm/lib/Target/VE/VVPNodes.inc
1 ↗(On Diff #306438)

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
simoll marked an inline comment as done.Nov 23 2020, 12:44 AM
simoll added inline comments.
llvm/lib/Target/VE/VEISelLowering.cpp
1678

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.

llvm/lib/Target/VE/VEInstrInfo.td
248

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?

llvm/lib/Target/VE/VVPInstrPatternsVec.td
36

Can we do 2), please? :) It makes sense on so many accounts

llvm/lib/Target/VE/VVPNodes.inc
1 ↗(On Diff #306438)

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.

kaz7 added inline comments.Nov 23 2020, 1:16 AM
llvm/lib/Target/VE/VEISelLowering.cpp
1678

Using 1 here is fine to me.

llvm/lib/Target/VE/VEInstrInfo.td
248

I have no real opinion, but suggest for a lower-case name for the consistency.

llvm/lib/Target/VE/VVPInstrPatternsVec.td
36

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.

llvm/lib/Target/VE/VVPNodes.inc
1 ↗(On Diff #306438)

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.

simoll updated this revision to Diff 306990.Nov 23 2020, 1:49 AM
simoll marked an inline comment as done.
  • Simplified macros
  • Keeping the vector inst+imm isel patterns and tests for now. When vector-immediate folding is upstream, we can remove that code
simoll updated this revision to Diff 307027.Nov 23 2020, 3:27 AM
simoll marked 3 inline comments as done.

Removed immediate variants in isel and tests. Added comment for future immediate folding.

kaz7 added a comment.Nov 23 2020, 3:58 AM

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

llvm/lib/Target/VE/VEISelLowering.cpp
259

Indentation.

llvm/lib/Target/VE/VEInstrInfo.td
248

Indentation before :

249

Additional empty line. It's OK if you intended it.

2229

Over 80 columns. Following several lines also has over 80 columns.

llvm/lib/Target/VE/VVPInstrInfo.td
2

80 columns.

11

Over 80 columns.

26

Over 80 columns.

llvm/lib/Target/VE/VVPInstrPatternsVec.td
21

Over 80 columns. Following lines also has over 80 columns.

llvm/test/CodeGen/VE/Vector/vec_add.ll
77

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.

kaz7 added inline comments.Nov 23 2020, 4:34 AM
llvm/test/CodeGen/VE/Vector/vec_add.ll
77

Just want to make sure that you intend it.

I meant that "Just want to make clear what you intend to do."

simoll updated this revision to Diff 307054.Nov 23 2020, 5:30 AM
simoll marked 12 inline comments as done.

NFC, Adressed comments

llvm/test/CodeGen/VE/Vector/vec_add.ll
77

Not testing the immediate special case here is intentional since there neither is a code path to test - i think the test-cases-with-immediate should be added here with the patch for immediate folding pass.

kaz7 accepted this revision.Nov 23 2020, 6:12 AM

LGTM.

This revision is now accepted and ready to land.Nov 23 2020, 6:12 AM
simoll updated this revision to Diff 307066.Nov 23 2020, 6:35 AM

NFC. Attaching this here because the rebase required manual merging.

This revision was landed with ongoing or failed builds.Nov 23 2020, 8:17 AM
This revision was automatically updated to reflect the committed changes.