This patch is the first in a series that derives from https://reviews.llvm.org/D25580 which will be abandoned because it is excessively complex.
This particular patch provides SDAG patterns for the nodes and adds the logic to decide when to expand the node vs. treating it as legal and using the SDAG patterns.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
674–676 | I have added a similar logic in one of the patches that I committed recently. Please make sure that we do not do this twice when you commit your code. (mine is a little bit earlier in the file IIRC) | |
7214–7217 | Do we need a lambda that is called only once? Since this function is very long already, I think creating a separate function for this is reasonable to avoid making this function too long. |
lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
674–676 | OK, thanks for mentioning that. I'll double check. | |
7214–7217 | I only added a lambda for two reasons:
But I really don't feel strongly about that reasoning and would be happy to change it to a function if that's what is preferred. What do the others think? |
lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
7214–7217 | No strong opinion here. I'm slightly more likely to want to outline code because it is a very long function, but no strong opinion. |
Can you please add some more comments in the summary about the intention of this group of patches?
lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
7203 | Would it be better to add this check in the condition above? | |
7267 | I don't follow this logic. Doesn't the isBuildVectorAllOnes already account for any Undefs in the vector? | |
lib/Target/PowerPC/PPCInstrVSX.td | ||
1383 | Is there a way we can do this without the use of AddedComplexity? | |
2752 | Please convert to C++ comments |
lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
7203 | Do you mean add this condition to the outer if? I don't think that would have the correct semantics. // BUILD_VECTOR nodes that are not constant splats of up to 32-bits can be // lowered to VSX instructions under certain conditions. // Without VSX however, there is no pattern more efficient than expanding the node. | |
7214–7217 | Two reviewers constitute a consensus in my opinion. I'll convert this to a static function. | |
7267 | Neither condition is strictly a strengthened version of the other. Think about a build vector node such as this: (v16i8 (build_vector i8:3, i8:3, i8:3, i8:undef, i8:undef, i8:3, ...)) That one has undefs and the splat size is 1 byte but it is not a BUILD_VECTOR of all ones. Although it is certainly possible for a node to have undefs and be a BUILD_VECTOR of all ones. | |
lib/Target/PowerPC/PPCInstrVSX.td | ||
1383 | Are you suggesting that we do away with AddedComplexity from VSX instruction definitions altogether? This was just added for consistency as it was initially missing. Ultimately, having it there doesn't change anything (at this time) because these instructions match PPCISD nodes that are not matched by anything else. However, in the highly unlikely situation where some VMX instructions are added in the future that match these nodes, it is good to have the AddedComplexity there as it appears in the remainder of the VSX target definition file. If you want, I can certainly remove this instance of it without affecting anything this patch does. | |
2752 | OK, will do. |
Perhaps it was an error to put the full description of the reasoning behind all of the patches in to the very last patch rather than the first...
Does the description in https://reviews.llvm.org/D26066 suffice for what you're after or would you like further commentary in the patch or code?
Removed an unnecessary lambda and converted it to a static function.
Simplified the logic.
Converted comments as requested.
lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
7078–7079 | The current comment says that this can be done for non constant splats, but this seems to indicate it cannot. | |
7082 | I don't think I've ever seen \param used like this. \param V The BuildVectorSDNode to analyze Also, as a minor nit, I find the description of the logic here a bit convoluted. I generally prefer more high-level english descriptions for the logic:
This needs to be refined somewhat, because I don't think it actually matches the logic in the function, but hopefully you get the idea. | |
7094 | This also looks a bit suspicious, but that's mostly because I'm not clear on the exact conditions under which we have efficient patterns. | |
7203 | Yup, this is good. Thanks. | |
7267 | I still don't follow the logic. In the example above, do we want to match it or not? Based on the comments above, we want to (all elements are either 3, or undef). How do we guarantee that all the all elements are the same constant - I don't see that check anywhere. | |
lib/Target/PowerPC/PPCInstrVSX.td | ||
1383 | My (slight) preference is to not use it unless it is absolutely necessary. |
lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
7078–7079 | This function is only called if the node was already confirmed not to be a constant splat (see below). So if the node is constant, it's building a vector out of different constants and it is beneficial to expand the BUILD_VECTOR so we can just get a LOAD node (from the constant pool). | |
7082 | To be perfectly honest with you, I'm not very familiar with Doxygen and this was based on a very quick reading about how these are to be written in LLVM code (so is probably wrong). I'll just follow the pattern you suggested. How about this for the description: // There are some patterns where it is beneficial to keep a BUILD_VECTOR // node as a BUILD_VECTOR node rather than expanding it. The patterns where // the opposite is true (expansion is beneficial) are: // The node builds a vector out of integers that are not 32 or 64-bits // The node builds a vector out of constants // The node is a "load-and-splat" // In all other cases, we will choose to keep the BUILD_VECTOR. | |
7094 | I think hopefully the updated comment will clarify this. |
lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
7078–7079 | That's probably worth an inline comment ;) | |
7082 | This looks fine. The only minor comment would be to indent and bullet/number the 3 lines that make up the list. That makes them standout visually. You also need to use /// (3 slashes) to indicate it's something doxygen can format. | |
7094 | Yes, this is good now. |
lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
7082 | Side note: the use of \brief is deprecated in llvm, we turned on autobrief some time ago. (Ref: http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments ) |
lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
7078–7079 | OK, I'll add that. // This function is called in a block that confirms the node is not a constant splat. // So a constant BUILD_VECTOR here means the vector is built out of different constants. | |
7082 | OK, thanks. I'll format it accordingly. | |
7267 | So at this point, we have determined that this is a splat of a constant and that we can build this vector by splatting a 1-byte value. We also have the value of that constant (SplatBits). However, the actual node may have some undefs. What this code will do is change any inputs into the value that needs to be splat to build this vector. In the example above, we'll just replace all inputs with constant 3 and leave everything the same so the BUILD_VECTOR node will be matched in the .td file. | |
lib/Target/PowerPC/PPCInstrVSX.td | ||
1383 | Yeah, for sure. I'll remove this. Direct moves do not have a corresponding VMX alternative so this accomplishes nothing. |
LGTM
lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
7082 | @mehdi_amini Thanks for the pointer - I didn't realize this! Although, it's not immediately obvious (to me at least) from that text that \brief has been deprecated in favour of autobrief. How does one go about updating that document? |
lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
7082 | Send a patch for llvm/docs/CodingStandards.rst |
I have added a similar logic in one of the patches that I committed recently. Please make sure that we do not do this twice when you commit your code. (mine is a little bit earlier in the file IIRC)