This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] NFCI: Choose consistent naming for predicated SDAG nodes
ClosedPublic

Authored by sdesmalen on Jun 15 2020, 9:33 AM.

Details

Summary

This patch proposes a naming convention for operations that take
a general predicate (and are thus predicated) that specifies
what happens to the false lanes.

Currently the _PRED suffix is used, which doesn't really say much other
than that it takes a predicate. In some instances this means it has
merging predication and in other cases it means zeroing-predication.

This patch also changes the order of operands to
AArch64ISD::DUP_MERGE_PASSTHRU, to pass the predicate as the first
operand, which is in line with all other predicates nodes. It takes the
passthru value as an explicit passthru value, which is always passed as
the last operand.

Diff Detail

Event Timeline

sdesmalen created this revision.Jun 15 2020, 9:33 AM
Herald added a project: Restricted Project. · View Herald Transcript
paulwalker-arm added inline comments.Jun 15 2020, 9:54 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.h
32

Nothing really against the names, being short is certainly a benefit over my next suggestion.

Is is worth being explicit when it comes to merging. For example _MERGE_OP1 and _MERGE_ZERO. Here MERGE implies a predicated operation and it's clear from the name where the results for inactive lanes comes from. Here I'm referring to _PRED_PASSTHRU where such things are not clear. In essence I'm suggesting forcing people to clearly state where the passthrough value comes from in all cases.

38

Given the above I'm wondering if this needs a special name, and if _PRED can be used for these cases as well as you describe below. I don't think there's any overlap so it's meaning should be clear.

sdesmalen updated this revision to Diff 273756.Jun 26 2020, 9:18 AM

Renamed nodes as follows:

// For predicated nodes where the result is a vector, the operation is
// controlled by a governing predicate and the inactive lanes are explicitly
// defined with a value, please stick the following naming convention:
//    _MERGE_OP0
//    _MERGE_OP1
//         :
//    _MERGE_OP<n>     The result value is a vector with false lanes equal to
//                     source operand OP<n>.
//
//    _MERGE_ZERO      The result value is a vector with false lanes actively
//                     zeroed.
//
// For other cases where no explicit action is needed to set the inactive lanes,
// or when the result is not a vector and it is needed or helpful to
// distinguish a node from similar unpredicated nodes, use:
//
//    _PRED
sdesmalen updated this revision to Diff 273832.Jun 26 2020, 2:08 PM
sdesmalen retitled this revision from [AArch64][SVE] NFC: Choose consistent naming for predicated SDAG nodes to [AArch64][SVE] NFCI: Choose consistent naming for predicated SDAG nodes.
sdesmalen edited the summary of this revision. (Show Details)

Following offline discussion with @paulwalker-arm, I've renamed DUP_MERGE_OP0 to DUP_MERGE_PASSTHRU and changed order of its operands to take the predicate as the first operand, which is more in line with the other predicated nodes. This consistency is useful for lowering to predidcated ops, like done in D82483. This means the patch isn't strictly NFC anymore though.

paulwalker-arm accepted this revision.Jun 27 2020, 1:26 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1300

Should these be aligned with the case statements? Although I guess this is just where clang-format wants to put them?

llvm/lib/Target/AArch64/AArch64ISelLowering.h
32

With the DUP change, can you remove _MERGE_OP0 when pushing the patch.

This revision is now accepted and ready to land.Jun 27 2020, 1:26 AM
This revision was automatically updated to reflect the committed changes.