This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Improvements for BUILD_VECTOR Vol. 1
ClosedPublic

Authored by nemanjai on Oct 24 2016, 9:59 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai updated this revision to Diff 75600.Oct 24 2016, 9:59 AM
nemanjai retitled this revision from to [PowerPC] Improvements for BUILD_VECTOR Vol. 1.
nemanjai updated this object.
nemanjai set the repository for this revision to rL LLVM.
nemanjai added a subscriber: llvm-commits.
amehsan added inline comments.Oct 26 2016, 7:23 AM
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.

nemanjai added inline comments.Oct 26 2016, 7:48 AM
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:

  1. The logic is very close to where it's used
  2. The logic isn't needed anywhere else

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?

echristo added inline comments.Oct 31 2016, 2:38 PM
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.

kbarton edited edge metadata.Oct 31 2016, 5:19 PM

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?
Then at least the remaining logic might be able to lower this, instead of just punting to the default case. If not, then I think a comment here is justified.

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

nemanjai added inline comments.Nov 1 2016, 2:38 AM
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.
We don't want to bail on constant splats regardless of whether we have VSX or not. And if this is not a constant splat, we may be able to do something better depending on a number of conditions - however, without VSX, we can't do anything very productive so we bail. How about the following comment:

// 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.

Can you please add some more comments in the summary about the intention of this group of patches?

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?

nemanjai updated this revision to Diff 76535.Nov 1 2016, 3:40 AM
nemanjai edited edge metadata.

Removed an unnecessary lambda and converted it to a static function.
Simplified the logic.
Converted comments as requested.

kbarton added inline comments.Nov 9 2016, 11:44 AM
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.
Typically you have a more detailed description following the \brief description, followed by a list of the parameters and return. Something like:

\param V The BuildVectorSDNode to analyze
\param HasDirectMove - indicates whether the DirectMove instruction is available
\returns true if there is a pattern in a .td file for this node, false otherwise.

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:
There are several efficient patterns for BUILD_VECTORS. If it is a constant splat it is handled <blah>.
If it is a non-constant splat, the following conditions must be true:

  1. It is not a load-and-splat
  2. It is either a floating point vector or an integer vector on a target with direct moves.

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.
Ideally we'll get remove the need for it at some point, but that is a big piece of work. For now, I don't think we should be using it unless it's absolutely necessary.

nemanjai added inline comments.Nov 10 2016, 7:49 AM
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.

kbarton added inline comments.Nov 10 2016, 11:54 AM
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.
And make sure to keep the \brief at the beginning - that is important (and quite useful).

7094

Yes, this is good now.

mehdi_amini added inline comments.Nov 10 2016, 12:06 PM
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 )

nemanjai added inline comments.Nov 10 2016, 12:36 PM
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.

kbarton accepted this revision.Nov 10 2016, 1:01 PM
kbarton edited edge metadata.

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?

This revision is now accepted and ready to land.Nov 10 2016, 1:01 PM
mehdi_amini added inline comments.Nov 10 2016, 1:02 PM
lib/Target/PowerPC/PPCISelLowering.cpp
7082

Send a patch for llvm/docs/CodingStandards.rst

nemanjai closed this revision.Nov 29 2016, 8:23 AM

Committed revision 288152.