This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Add getBuildVector and getSplatBuildVector helpers.
ClosedPublic

Authored by ab on Feb 11 2016, 4:28 PM.

Details

Summary

Hopefully straightforward, what do you think?

I was mostly annoyed by the awkwardness of building a splat, but figured I might as well add getBuildVector.

I thought I'd wait for opinions before finishing to change the remaining calls.

Diff Detail

Repository
rL LLVM

Event Timeline

ab updated this revision to Diff 47746.Feb 11 2016, 4:28 PM
ab retitled this revision from to [CodeGen] Add getBuildVector and getSplatBuildVector helpers..
ab updated this object.
ab added reviewers: spatel, RKSimon, arsenm.
ab added a subscriber: llvm-commits.
ab updated this revision to Diff 47752.Feb 11 2016, 4:35 PM

Correct for overzealous macro. Remove now dead variable.

RKSimon edited edge metadata.Feb 11 2016, 5:06 PM

Very useful!

include/llvm/CodeGen/SelectionDAG.h
588 ↗(On Diff #47752)

Please try to consistent with the DL / VT arg order with other DAG node creators - most seem to be VT and then DL.

592 ↗(On Diff #47752)

It'd be great if you could add extra tests here - for instance all Ops must be the same valuetype (if float they must match VT.getScalarType() - if integer they can be greater than or equal to the number scalar bits).

lib/Target/X86/X86ISelLowering.cpp
4539 ↗(On Diff #47752)

Why not updated to DAG.getBuildVector?

spatel edited edge metadata.Feb 12 2016, 7:00 AM

I didn't look closely enough to know if this would help here, but it could shave a couple of lines off of the current D17181 patch: DAG.getConstant() has the unadvertised feature (no documentation or implementation comments) of creating splat build vector constants.

I used it here (hopefully without introducing any bugs!):
http://reviews.llvm.org/rL260609

ab added a comment.Feb 12 2016, 5:36 PM

I didn't look closely enough to know if this would help here, but it could shave a couple of lines off of the current D17181 patch: DAG.getConstant() has the unadvertised feature (no documentation or implementation comments) of creating splat build vector constants.

Interesting, I didn't know! Let me add a comment and use that instead.

I do think that the name "getConstant" could be confusing. In practice I don't think it's a problem, with proper names and context. That's not ideal though, so perhaps we should add "getConstantVec" (or "getConstantSplat"?) versions of the 6 getConstant variants we already have?

ab added inline comments.Feb 12 2016, 5:39 PM
lib/Target/X86/X86ISelLowering.cpp
4539 ↗(On Diff #47752)

Because the array is made of SDUse, not SDValues.

The formatting change betrays me undoing my macro when realizing that this didn't build anymore ;)

Hi Ahmed, do you still intend to work on this?

I still think this would be useful - especially as BUILD_VECTOR with different sized operands don't assert until much later without it.

ab added a comment.Mar 28 2016, 11:20 AM

Hi Ahmed, do you still intend to work on this?

I still think this would be useful - especially as BUILD_VECTOR with different sized operands don't assert until much later without it.

Yes, sorry, I've been busy with non-llvm things; I'll try to get back to this in the next few days!

ab updated this revision to Diff 52648.Apr 4 2016, 5:57 PM
ab edited edge metadata.
  • Rebase
  • Update all users
  • Remove the checks: turns out they're in VerifySDNode anyway. I'd move them to getBuildVector, but that misses all the getNode calls. Then again that might be a good reason to use getBuildVector in the first place.
ab marked an inline comment as done.Apr 4 2016, 6:00 PM
ab added inline comments.
include/llvm/CodeGen/SelectionDAG.h
641 ↗(On Diff #52648)

What do you think of moving the VerifySDNode checks here?

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
3294–3299 ↗(On Diff #52648)

I think this block is unreachable: if two operands are constants, there's no way for this op to be vector.

If I didn't miss anything, I'll remove this separately.

RKSimon added inline comments.Apr 6 2016, 2:54 AM
include/llvm/CodeGen/SelectionDAG.h
641 ↗(On Diff #52648)

I'm happy for them to stay where they are as long as:

(a) there isn't any way that the asserts can be avoided depending on the manner in which BUILD_VECTOR is created.

(b) the assert is directly in line in the call stack with whatever created the BUILD_VECTOR in the first place - we used to have cases where only a much later assert would fire regarding the inconsistent valuetypes leaving us with no idea what generated the faulty BUILD_VECTOR in the first place.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
3294–3299 ↗(On Diff #52648)

LCOV agrees in that no tests ever hit it.

ab marked 2 inline comments as done.Apr 12 2016, 11:21 AM
ab added inline comments.
include/llvm/CodeGen/SelectionDAG.h
641 ↗(On Diff #52648)

Yes, any new node goes through InsertNode and VerifySDNode; this is what a failed assert looks like:

9 llc 0x0000000111bf8e22 VerifySDNode(llvm::SDNode*) + 1250
10 llc 0x0000000111bf88f8 llvm::SelectionDAG::InsertNode(llvm::SDNode*) + 88
11 llc 0x0000000111c05746 llvm::SelectionDAG::getNode(unsigned int, llvm::SDLoc, llvm::EVT, llvm::SDValue, llvm::SDValue, llvm::SDNodeFlags const*) + 17414
12 llc 0x0000000111c07c75 llvm::SelectionDAG::getNode(unsigned int, llvm::SDLoc, llvm::EVT, llvm::ArrayRef<llvm::SDValue>, llvm::SDNodeFlags const*) + 661
13 llc 0x0000000111c1da85 llvm::SelectionDAG::getBuildVector(llvm::EVT, llvm::SDLoc, llvm::ArrayRef<llvm::SDValue>) + 133

I can imagine one case where this is a problem though: if we ever add code to getNode that folds BUILD_VECTORs, that code will execute before the checks.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
3294–3299 ↗(On Diff #52648)

r266100

ab updated this revision to Diff 53435.Apr 12 2016, 11:23 AM
ab edited edge metadata.
ab marked an inline comment as done.

Rebased

RKSimon added inline comments.Apr 13 2016, 9:59 AM
include/llvm/CodeGen/SelectionDAG.h
636 ↗(On Diff #53435)

Integer operand valuetypes must be GREATER than or equal to the vector.

ab updated this revision to Diff 53588.Apr 13 2016, 10:32 AM

Comment fix: smaller -> wider type

ab added a comment.Apr 24 2016, 12:41 PM

Good to go?

RKSimon accepted this revision.Apr 26 2016, 10:54 AM
RKSimon edited edge metadata.

LGTM - in hindsight the getSplatBuildVector looks like the more useful one of the two, but getBuildVector does at least reduce the code text a little to allow clang-format to tidy things up a bit ;-)

This revision is now accepted and ready to land.Apr 26 2016, 10:54 AM
This revision was automatically updated to reflect the committed changes.