[llvm-commits] [llvm] r65296 - in /llvm/trunk: include/llvm/CodeGen/ lib/CodeGen/SelectionDAG/ lib/Target/CellSPU/ lib/Target/PowerPC/ lib/Target/X86/ test/CodeGen/X86/

Chris:

I did float this by the dev list first a couple of weeks ago, didn’t receive any comments. It’s not entirely gratuitous; the rationale for adding a new node class is threefold:

a) Convenience for the backends. Since it benefits multiple backends (PPC and CellSPU), it’s a logical addition. I reckon the GPU efforts would also benefit.
b) Where else would one encapsulate a constant splat predicate? SelectionDAG and SDNode are not good classes for constant splat detection, since it’s functionality specific to building vectors.
c) Future work. At some point (or another), having target-specific SDNode polymorphism is an issue that has to be addressed, in light of the vector swizzling support conversation thread. One wants the benefits of what the legalization and DAG combiner phases provide, but not have to add more code for a target-specific ISD enumeration value with a generic SDNode, if that functionality is already available (and doesn’t need to be overridden.)

It seems to me that LLVM will eventually need to have a bijective mapping between the ISD enumeration and the SDNodes, in the case of (c). (c) is a huge undertaking and needs careful thought. For example, the whole “Is the node legal, custom, promotable or expanded” testing screams for static protocol methods hooked to per-target protected virtual methods.

But (a) and (b) are the dominant factors for contributing the change. It’s not completely altruistic: the commit makes it considerably easier for the CellSPU backend to deal with various splats at instruction selection rather than during target-dependent lowering.

I attempted to be pretty thorough with the change before committing. I ran through the tests before committing. The changes were primarily method invocations, which admittedly were pretty numerous. It wasn’t as if I was adding DebugLoc and changing prototypes everywhere. :slight_smile:

Ok, I’m sure there were places in clang and llvm-gcc-4.2 that I overlooked, and I appologize for being narrowly focused on the backend infrastructure.

-scooter

Chris:

I did float this by the dev list first a couple of weeks ago, didn't receive any comments.

Ok, I didn't see it, sorry about that.

It's not entirely gratuitous; the rationale for adding a new node class is threefold:

a) Convenience for the backends. Since it benefits multiple backends (PPC and CellSPU), it's a logical addition. I reckon the GPU efforts would also benefit.

I don't see this. Adding some helper methods would have the same functionality.

b) Where else would one encapsulate a constant splat predicate? SelectionDAG and SDNode are not good classes for constant splat detection, since it's functionality specific to building vectors.

Eventually we need to add a ShuffleSDNode class for other reasons. Parking it on SelectionDAG or SDNode is not a good place to put it.

c) Future work. At some point (or another), having target-specific SDNode polymorphism is an issue that has to be addressed, in light of the vector swizzling support conversation thread. One wants the benefits of what the legalization and DAG combiner phases provide, but not have to add more code for a target-specific ISD enumeration value with a generic SDNode, if that functionality is already available (and doesn't need to be overridden.)

This should be addressed in different ways.

I didn't go into it in the previous email, but I am strongly against several engineering issues in your patch. "Remembering" whether something is a splat or not (for example) in the node is unnecessary and a really dangerous thing to do. It is unnecessary because it can be computed on demand (as we do now) and it is dangerous, because it means that a) clients have to keep it up to date as they change the shuffle mask, and b) the nodes need to be reCSEd as the operands are changed.

I attempted to be pretty thorough with the change before committing. I ran through the tests before committing. The changes were primarily method invocations, which admittedly were pretty numerous. It wasn't as if I was adding DebugLoc and changing prototypes everywhere. :slight_smile:

I really appreciate your thoroughness and the fact that the patch apparently didn't break anything.

In my opinion, the proper direction for shuffles is:

1. Back out your patch.
2. Move the functionality of "is splat" etc to method somewhere, e.g. on SDNode.
3. Introduce a new ShuffleVectorSDNode that only has two SDValue operands (the two input vectors), but that also contains an array of ints in the node (not as operands).
4. Move the helper functions from #2 back into ShuffleVectorSDNode.

The important part of #3 is that we really want an array of ints (using -1 for undef) for the shuffle mask, not "operands". This eliminates the nastiness we have now were we need a buildvector, and it eliminates the dance we have to prevent the build vector from being legalized, and prevent the integer operands to it from being legalized.

Your patch doesn't get us further towards this eventual design point, which is why I prefer it to be reverted.

-Chris

Chris:

I did float this by the dev list first a couple of weeks ago, didn’t receive any comments.

Ok, I didn’t see it, sorry about that.

It happens. :slight_smile:

a) Convenience for the backends. Since it benefits multiple backends (PPC and CellSPU), it’s a logical addition. I reckon the GPU efforts would also benefit.

It’s not entirely gratuitous; the rationale for adding a new node class is threefold:

I don’t see this. Adding some helper methods would have the same functionality.

And the first thing the helper method would have to check is if this SDNode is a BUILD_VECTOR node, right?

b) Where else would one encapsulate a constant splat predicate? SelectionDAG and SDNode are not good classes for constant splat detection, since it’s functionality specific to building vectors.

Eventually we need to add a ShuffleSDNode class for other reasons. Parking it on SelectionDAG or SDNode is not a good place to put it.

Ok, so that’s a subclass of BuildVectorSDNode or the ShuffleSDNode class is passed one or more BuildVectorSDNode operands (vide infra.)

c) Future work. At some point (or another), having target-specific SDNode polymorphism is an issue that has to be addressed, in light of the vector swizzling support conversation thread. One wants the benefits of what the legalization and DAG combiner phases provide, but not have to add more code for a target-specific ISD enumeration value with a generic SDNode, if that functionality is already available (and doesn’t need to be overridden.)

This should be addressed in different ways.

I didn’t go into it in the previous email, but I am strongly against several engineering issues in your patch. “Remembering” whether something is a splat or not (for example) in the node is unnecessary and a really dangerous thing to do. It is unnecessary because it can be computed on demand (as we do now) and it is dangerous, because it means that a) clients have to keep it up to date as they change the shuffle mask, and b) the nodes need to be reCSEd as the operands are changed.

I can take the result caching out, seemed to be a good idea at the time. The predicate is only called in a couple of places and I was prematurely optimizing for the case when it’s called repeatedly.

However, (b) is something that is handled elsewhere. BuildVectorSDNode is merely a container class – the constructor isn’t doing anything funky other than calling the base constructor with the appropriate SDOperand list and size. I seem to recall that calling UpdateNodeOperands is generally frowned upon, but I’m happy to look into that particular issue. But at this point, I didn’t see any place where a BUILD_VECTOR has its operands updated. Removing the result caching would avoid bugs, with which I agree.

In my opinion, the proper direction for shuffles is:

  1. Back out your patch.

Before I back it out, please read my comments below carefully. :slight_smile:

  1. Move the functionality of “is splat” etc to method somewhere, e.g. on SDNode.
  2. Introduce a new ShuffleVectorSDNode that only has two SDValue operands (the two input vectors), but that also contains an array of ints in the node (not as operands).

There are two cases: a constant vector and a vector shuffle (moving two vectors around with a constant mask, which itself is a constant vector.) I’m not seeing how a splat is equivalent to a shuffle (unless the second input operand is totally undef and the mask has a magic pattern, which I think confuses more than clarifies.) On a pure vector machine, like the CellSPU, splats and shuffles are not equivalent concepts (*)

The important part of #3 is that we really want an array of ints (using -1 for undef) for the shuffle mask, not “operands”. This eliminates the nastiness we have now were we need a buildvector, and it eliminates the dance we have to prevent the build vector from being legalized, and prevent the integer operands to it from being legalized.

Agreed, and I can see why one would want to have an array of instead of operands to build the shuffle mask. The mask is generally a byte array, at least for CellSPU and PPC, but could be a bitmask (cell has one of those too.) I think you’re still left with a constant formation problem in the case of a constant vector that is separate from vector shuffles.

Incidentally, -1 is a perfectly legal value for a CellSPU shuffle mask in the byte array (actually, it’s 0b1xxxxxxxx, but that doesn’t mean it can’t or won’t occur.)

-scooter

(*) To be fair, shuffles are part of i64 constant formation on CellSPU for special values of the mask.

I'm working on #2 and #3 right now, and hope to land something in the next couple days.

Nate

Details, since this has implications for vector machine backends?

I’m just not groking how moving isSplat to SDNode eliminates BUILD_VECTOR or deals with constant vector formation. I like the idea of a ShuffleVectorSDNode (moves things in the direction of a bijective ISD to SDNode mapping), but eliminating BuildVectorSDNode in its entirety doesn’t deal with constant and variable vectors.

-scooter

And the first thing the helper method would have to check is if this SDNode is a BUILD_VECTOR node, right?

Right. It's really not much different than what you have now, just moving the point where you check. In your code right now, when you want to call your isConstantSplat method, you first dyn_cast the node to a BuildVectorSDNode. Just move the check inside isConstantSplat.

b) Where else would one encapsulate a constant splat predicate? SelectionDAG and SDNode are not good classes for constant splat detection, since it's functionality specific to building vectors.

Eventually we need to add a ShuffleSDNode class for other reasons. Parking it on SelectionDAG or SDNode is not a good place to put it.

Ok, so that's a subclass of BuildVectorSDNode or the ShuffleSDNode class is passed one or more BuildVectorSDNode operands (vide infra.)

There are a couple of different issues getting combined here. Changing the vector shuffles (as Nate is apparently already doing) is related in some ways to what you're doing with BUILD_VECTORS but it might be better to consider them as separate issues. From what I can see, the point of your BuildVectorSDNode patch was simply to share code across target backends. That doesn't have anything to do with vector shuffles.

c) Future work. At some point (or another), having target-specific SDNode polymorphism is an issue that has to be addressed, in light of the vector swizzling support conversation thread. One wants the benefits of what the legalization and DAG combiner phases provide, but not have to add more code for a target-specific ISD enumeration value with a generic SDNode, if that functionality is already available (and doesn't need to be overridden.)

This should be addressed in different ways.

I didn't go into it in the previous email, but I am strongly against several engineering issues in your patch. "Remembering" whether something is a splat or not (for example) in the node is unnecessary and a really dangerous thing to do. It is unnecessary because it can be computed on demand (as we do now) and it is dangerous, because it means that a) clients have to keep it up to date as they change the shuffle mask, and b) the nodes need to be reCSEd as the operands are changed.

I can take the result caching out, seemed to be a good idea at the time. The predicate is only called in a couple of places and I was prematurely optimizing for the case when it's called repeatedly.

However, (b) is something that is handled elsewhere. BuildVectorSDNode is merely a container class -- the constructor isn't doing anything funky other than calling the base constructor with the appropriate SDOperand list and size. I seem to recall that calling UpdateNodeOperands is generally frowned upon, but I'm happy to look into that particular issue. But at this point, I didn't see any place where a BUILD_VECTOR has its operands updated. Removing the result caching would avoid bugs, with which I agree.

Once you remove the caching, there's not much left of BuildVectorSDNode, is there? You might as well just add SDNode::isConstantSplat.

In my opinion, the proper direction for shuffles is:

  1. Back out your patch.
  2. Move the functionality of “is splat” etc to method somewhere, e.g.
    on SDNode.
  3. Introduce a new ShuffleVectorSDNode that only has two SDValue
    operands (the two input vectors), but that also contains an array of
    ints in the node (not as operands).
  4. Move the helper functions from #2 back into ShuffleVectorSDNode.

I’m working on #2 and #3 right now, and hope to land something in the next couple days.

Details, since this has implications for vector machine backends?

It’s basically as Chris said; there will be a ShuffleVectorSDNode, and appropriate helper functions, node profile, and DAGCombiner support.

I’m just not groking how moving isSplat to SDNode eliminates BUILD_VECTOR or deals with constant vector formation.

You can have a static method on SDNode that took an SDNode, checked if it was a build vector, and calculated whatever splat information you wanted. There’s no need for BuildVectorSDNode for this particular functionality.

I like the idea of a ShuffleVectorSDNode (moves things in the direction of a bijective ISD to SDNode mapping), but eliminating BuildVectorSDNode in its entirety doesn’t deal with constant and variable vectors.

LLVM has no variable shuffle instruction, so there’s no variable vectors to deal with; moving the constants into the shuffle sdnode does address constant shuffles (which are all of them). Variable shuffles would need to be represented as target nodes at present, and my work will not change that.

Nate

And the first thing the helper method would have to check is if this
SDNode is a BUILD_VECTOR node, right?

Right. It’s really not much different than what you have now, just
moving the point where you check. In your code right now, when you
want to call your isConstantSplat method, you first dyn_cast the node
to a BuildVectorSDNode. Just move the check inside isConstantSplat.

I got that. LLVM will need to support some class that encapsulates constant vectors. It might suffice to say that at this point, what is ISD::BUILD_VECTOR should really become the equivalent of ISD::Constant for vectors. I could refine my patch so that this particular functionality is implemented. But I’m really not convinced that moving a constant splat predicate to SDNode is good O-O software design. Really - Is SDNode the right place to look for a constant splat predicate?

dyn_cast is generally unavoidable when you need to get at the subclass, i.e. ConstantSDNode and any other node subclassed off SDNode. I’m not entirely buying that as rationale for killing the class and moving the predicate.

There are a couple of different issues getting combined here.
Changing the vector shuffles (as Nate is apparently already doing) is
related in some ways to what you’re doing with BUILD_VECTORS but it
might be better to consider them as separate issues. From what I can
see, the point of your BuildVectorSDNode patch was simply to share
code across target backends. That doesn’t have anything to do with
vector shuffles.

Part of the reason was consolidating code, partially for the convenience of the CellSPU backend where a lot of stuff now gets “lowered” during instruction selection.

At a larger perspective, shuffles and vector constants are two separate issues. I’m not clear why shuffles and constants were combined into the same thread. I’ve never proposed that shuffles and vector constants should be combined.

Once you remove the caching, there’s not much left of
BuildVectorSDNode, is there? You might as well just add
SDNode::isConstantSplat.

No, that’s not true: you lose an important feature of O-O software design, namely, encapsulation of the results of the constant splat predicate. isConstantSplat() produces a number of results, e.g., splat size, the actual splat bits. These would still be generated on the fly, even if the result of the previous invocation were no longer cached.

Which means, that if one moves the functionality to SDNode, all of these results have to be passed by reference. That’s generally the hallmark of not-so-desirable O-O software design.

I’m not saying we need to turn LLVM into the paragon of O-O-ness. But let’s use it when it’s advantageous.

To summarize:
a) Shuffles are not constants. The two are very separate.
b) BuildVectorSDNode should probably become a ConstantVectorSDNode, similar to ConstantSDNode, et. al.
c) Killing the class breaks good O-O design.

-scooter

It’s basically as Chris said; there will be a ShuffleVectorSDNode, and appropriate helper functions, node profile, and DAGCombiner support.

Fine. For vector shuffles. But again, what about vector constants, e.g., v4i32 <0, 1, 2, 3, 4>? BuildVectorSDNode is still a reasonable subclass to have for encapsulating constant vectors (should be renamed, but hey, it’s what it’s called today.)

You can have a static method on SDNode that took an SDNode, checked if it was a build vector, and calculated whatever splat information you wanted. There’s no need for BuildVectorSDNode for this particular functionality.

You’re talking about moving the functionality to a class where it makes no sense to look for it. That’s one issue where I disagree and would argue for good O-O software design. At least my patch puts the functionality in a place where it’s reasonable to expect it reside.

I like the idea of a ShuffleVectorSDNode (moves things in the direction of a bijective ISD to SDNode mapping), but eliminating BuildVectorSDNode in its entirety doesn’t deal with constant and variable vectors.

LLVM has no variable shuffle instruction, so there’s no variable vectors to deal with; moving the constants into the shuffle sdnode does address constant shuffles (which are all of them). Variable shuffles would need to be represented as target nodes at present, and my work will not change that.

What about this case where the arguments are entirely variable and you have no knowledge of their contents:

define <4 x i32> @v4i32_shuffle(<4 x i32> %a, <4 x i32> %b) nounwind readnone {
entry:
%tmp1 = shufflevector <4 x i32> %a, <4 x i32> %b, <4 x i32> < i32 1, i32 2, i32 3, i32 0 >
ret <4 x i32> %tmp1
}

It’s perfectly legit LLVM code and it’s a case where the two input operands are variables, not constants. Only the mask is a constant.

-scooter

I think what you meant to say here is that LLVM doesn’t deal with variable-length vectors and it does not deal with non-constant masks. That’s not the same what I was proposing, which is (now) an argument for O-O hygiene and creating constant vectors.

Somehow I managed to get the impression that Chris was proposing moving constant vector formation, e.g., v4i32 <0, 1, 2, 3, 4>, into the ShuffleVectorSDNode class. That seems really odd, but that’s how I read his four steps. Happy to be proven that this was a misperception.

It’s really in constant vector formation that isConstantSplat() is actually useful. I’m not sure how useful isContantSplat() will be when moved to a shuffle vector SDNode class.

-scooter

It’s basically as Chris said; there will be a ShuffleVectorSDNode, and appropriate helper functions, node profile, and DAGCombiner support.

Fine. For vector shuffles. But again, what about vector constants, e.g., v4i32 <0, 1, 2, 3, 4>? BuildVectorSDNode is still a reasonable subclass to have for encapsulating constant vectors (should be renamed, but hey, it’s what it’s called today.)

You’re talking about two very very very different things: shuffle_vector “masks” and constant vectors.

Constant vectors have to be legal for a target. This has implications for legalization and many other things. BUILD_VECTOR is fine for them.

Shuffle masks do not and should not be legalized as a build_vector. If you have a machine that has shuffles but has no support for forming a “buildvector” of a constant, you don’t want to end up with shufflevectors of loads of the mask from the constant pool.

These are very different issues and should be modeled differently.

You can have a static method on SDNode that took an SDNode, checked if it was a build vector, and calculated whatever splat information you wanted. There’s no need for BuildVectorSDNode for this particular functionality.

You’re talking about moving the functionality to a class where it makes no sense to look for it. That’s one issue where I disagree and would argue for good O-O software design. At least my patch puts the functionality in a place where it’s reasonable to expect it reside.

Moving these methods to SDNode was just a short term place to park them. You could also just make them be global functions for all I care. When Nate introduces a new shufflevector class in the next couple days (apparently), we’d want to move these to that class.

What about this case where the arguments are entirely variable and you have no knowledge of their contents:

define <4 x i32> @v4i32_shuffle(<4 x i32> %a, <4 x i32> %b) nounwind readnone {
entry:
%tmp1 = shufflevector <4 x i32> %a, <4 x i32> %b, <4 x i32> < i32 1, i32 2, i32 3, i32 0 >
ret <4 x i32> %tmp1
}

It’s perfectly legit LLVM code and it’s a case where the two input operands are variables, not constants. Only the mask is a constant.

Scott, we’re only talking about the shuffle mask here. Shuffle vector should always take two vectors as “operands”.

To summarize:
a) Shuffles are not constants. The two are very separate.

Yes, we agree on this.

b) BuildVectorSDNode should probably become a ConstantVectorSDNode, similar to ConstantSDNode, et. al.

I’m fine with having a BuildVectorSDNode class in principle, but I don’t like your current one for reasons I already mentioned (caching stuff that doesn’t need to be etc).

c) Killing the class breaks good O-O design.

buildvector should have nothing to do with shuffles. Before your patch we didn’t have “good OO design”. If you’d like to improve OO design, that is very welcome, but lets engineer it in a consistent direction.

-Chris

It’s basically as Chris said; there will be a ShuffleVectorSDNode, and appropriate helper functions, node profile, and DAGCombiner support.

Fine. For vector shuffles. But again, what about vector constants, e.g., v4i32 <0, 1, 2, 3, 4>? BuildVectorSDNode is still a reasonable subclass to have for encapsulating constant vectors (should be renamed, but hey, it’s what it’s called today.)

You’re talking about two very very very different things: shuffle_vector “masks” and constant vectors.

Yup. That’s why I was absolutely mystified when you proposed the following:

  1. Move the functionality of “is splat” etc to method somewhere, e.g. on SDNode.
  2. Introduce a new ShuffleVectorSDNode that only has two SDValue operands (the two input vectors), but that also contains an array of ints in the node (not as operands).
  3. Move the helper functions from #2 back into ShuffleVectorSDNode.

#4 is the part that completely mystifies me. I reckon that I could almost live with #2, as much I see it as being unsound software engineering and more of a performance optimization.

Constant vectors have to be legal for a target. This has implications for legalization and many other things. BUILD_VECTOR is fine for them.

Shuffle masks do not and should not be legalized as a build_vector. If you have a machine that has shuffles but has no support for forming a “buildvector” of a constant, you don’t want to end up with shufflevectors of loads of the mask from the constant pool.

Never disagreed with that point. But recall what you had previous written and quoted above. Hence my confusion when you wrote step #4 – that’s an even less logical place to look for isConstantSplat functionality and is really misplaced.

You can have a static method on SDNode that took an SDNode, checked if it was a build vector, and calculated whatever splat information you wanted. There’s no need for BuildVectorSDNode for this particular functionality.

You’re talking about moving the functionality to a class where it makes no sense to look for it. That’s one issue where I disagree and would argue for good O-O software design. At least my patch puts the functionality in a place where it’s reasonable to expect it reside.

Moving these methods to SDNode was just a short term place to park them. You could also just make them be global functions for all I care. When Nate introduces a new shufflevector class in the next couple days (apparently), we’d want to move these to that class.

The operands to shufflevector may be constant vectors, but that’s not where the test for a constant splat ought to be.

b) BuildVectorSDNode should probably become a ConstantVectorSDNode, similar to ConstantSDNode, et. al.

I’m fine with having a BuildVectorSDNode class in principle, but I don’t like your current one for reasons I already mentioned (caching stuff that doesn’t need to be etc).

Keeping track of results is a bad thing? Ok.

Removing the computedSplat bool is easy. And I could add the caveat that isConstantSplat should be called before any of the result state getter methods are called. I’m not sure that passing a bunch of references to a function is any more wholesome.

c) Killing the class breaks good O-O design.

buildvector should have nothing to do with shuffles. Before your patch we didn’t have “good OO design”. If you’d like to improve OO design, that is very welcome, but lets engineer it in a consistent direction.

buildvector never had anything to do with shuffles. The patch never had and never will. Somewhere along the lines I think you got confused.

But hey, you’re the boss. Will rip the patch out at earliest opportunity and live with the consequences, even if testing for constant splats ends up in a shufflevector class (see #4 above.)

-scooter

It’s basically as Chris said; there will be a ShuffleVectorSDNode, and appropriate helper functions, node profile, and DAGCombiner support.

Fine. For vector shuffles. But again, what about vector constants, e.g., v4i32 <0, 1, 2, 3, 4>? BuildVectorSDNode is still a reasonable subclass to have for encapsulating constant vectors (should be renamed, but hey, it’s what it’s called today.)

You’re talking about two very very very different things: shuffle_vector “masks” and constant vectors.

Yup. That’s why I was absolutely mystified when you proposed the following:

  1. Move the functionality of “is splat” etc to method somewhere, e.g. on SDNode.
  2. Introduce a new ShuffleVectorSDNode that only has two SDValue operands (the two input vectors), but that also contains an array of ints in the node (not as operands).
  3. Move the helper functions from #2 back into ShuffleVectorSDNode.

#4 is the part that completely mystifies me. I reckon that I could almost live with #2, as much I see it as being unsound software engineering and more of a performance optimization.

I don’t see how it is relevant. The representation of constants and splat masks should be complete different. If you want to see if a shuffle mask is a splat and/or if a constant vector is a splat, then you’d need different code for both.

c) Killing the class breaks good O-O design.

buildvector should have nothing to do with shuffles. Before your patch we didn’t have “good OO design”. If you’d like to improve OO design, that is very welcome, but lets engineer it in a consistent direction.

buildvector never had anything to do with shuffles. The patch never had and never will. Somewhere along the lines I think you got confused.

Entirely possible, that frequently happens! :slight_smile:

But hey, you’re the boss. Will rip the patch out at earliest opportunity and live with the consequences, even if testing for constant splats ends up in a shufflevector class (see #4 above.)

To be clear, I’m entirely in favor of improving support for both buildvector and shuffle. Most of my comments have been about shuffle, but improvements to introduce a new buildvectorsdnode class would also make sense… but they should be separate from shuffle stuff.

Thanks Scott!

-Chris

Because, Chris, these are your exact words – move the constant splat code to somewhere, e.g., on SDNode then move those (constant splat code) functions to ShuffleVectorSDNode. That’s how I read your steps. Hence my incredulity.

-scooter

3. Introduce a new ShuffleVectorSDNode that only has two SDValue
operands (the two input vectors), but that also contains an array of
ints in the node (not as operands).

...

The important part of #3 is that we really want an array of ints
(using -1 for undef) for the shuffle mask, not "operands". This
eliminates the nastiness we have now were we need a buildvector, and
it eliminates the dance we have to prevent the build vector from being
legalized, and prevent the integer operands to it from being legalized.

This is PR2957 (which originally suggested a variadic SDNode, but it
quickly became clear that an array of ints is better). It would be
great to have a volunteer for this (I don't have time).

Ciao,

Duncan.

Duncan:

I’m still stymied how this whole thread ended up about shuffle vector nodes, when the original problem was my build vector patch. I’m still working on backing the build vector patch out (it isn’t clean with all of the intervening commits and I have pressing management tasks which command my attention.)

-scooter

I believe this patch has broken a PPC app that I am tracking. Here is a reduced test case. Reproduce with llc -mattr=+Altivec -mcpu=g5. The backtrace looks like this:

#0 0x9333ae42 in __kill ()
#1 0x9333ae34 in kill$UNIX2003 ()
#2 0x933ad23a in raise ()
#3 0x933b9679 in abort ()
#4 0x933ae3db in __assert_rtn ()
#5 0x0008bd8f in llvm::MVT::getVectorElementType (this=0xbfffdda4) at ValueTypes.h:317
#6 0x002aed06 in BuildSplatI (Val=0, SplatSize=8, VT={{V = 24, SimpleTy = llvm::MVT::v4i32, LLVMTy = 0x18}}, DAG=@0x16088a0, dl={Idx = 4294967295}) at PPCISelLowering.cpp:311
5
#7 0x002afae4 in llvm::PPCTargetLowering::LowerBUILD_VECTOR (this=0x1803d58, Op={Node = 0x157a530, ResNo = 0}, DAG=@0x16088a0) at PPCISelLowering.cpp:3200
#8 0x002bb54f in llvm::PPCTargetLowering::LowerOperation (this=0x1803d58, Op={Node = 0x157a530, ResNo = 0}, DAG=@0x16088a0) at PPCISelLowering.cpp:3766
#9 0x0051bed6 in (anonymous namespace)::SelectionDAGLegalize::LegalizeOp (this=0xbffff0e8, Op={Node = 0x157a530, ResNo = 0}) at LegalizeDAG.cpp:1608
#10 0x0054837d in (anonymous namespace)::SelectionDAGLegalize::HandleOp (this=0xbffff0e8, Op={Node = 0x157a530, ResNo = 0}) at LegalizeDAG.cpp:519
#11 0x005485a5 in (anonymous namespace)::SelectionDAGLegalize::LegalizeDAG (this=0xbffff0e8) at LegalizeDAG.cpp:389
#12 0x00548734 in llvm::SelectionDAG::Legalize (this=0x16088a0, TypesNeedLegalizing=false, Fast=false) at LegalizeDAG.cpp:8648
#13 0x005ec313 in llvm::SelectionDAGISel::CodeGenAndEmitDAG (this=0x1608780) at SelectionDAGISel.cpp:626
#14 0x005ee7e2 in llvm::SelectionDAGISel::SelectBasicBlock (this=0x1608780, LLVMBB=0x1603fa0, Begin={<bidirectional_iteratorllvm::Instruction,int> = {<std::iterator<std::bid
irectional_iterator_tag,llvm::Instruction,int,llvm::Instruction*,llvm::Instruction&>> = {}, }, NodePtr = 0x1604dd0}, End={<bidirectional_iterat
orllvm::Instruction,int> = {<std::iteratorstd::bidirectional_iterator_tag,llvm::Instruction,int,llvm::Instruction*,llvm::Instruction&> = {}, <No data field\

}, NodePtr = 0x16049e0}) at SelectionDAGISel.cpp:500
#15 0x005ef123 in llvm::SelectionDAGISel::SelectAllBasicBlocks (this=0x1608780, Fn=@0x1603720, MF=@0x160d520, MMI=0x160bbd0, DW=0x1608fe0, TII=@0x1803ce0) at SelectionDAGISel.
cpp:856
#16 0x005efe54 in llvm::SelectionDAGISel::runOnFunction (this=0x1608780, Fn=@0x1603720) at SelectionDAGISel.cpp:327
#17 0x002a3aea in (anonymous namespace)::PPCDAGToDAGISel::runOnFunction (this=0x1608780, Fn=@0x1603720) at PPCISelDAGToDAG.cpp:54
#18 0x00874127 in llvm::FPPassManager::runOnFunction (this=0x1606610, F=@0x1603720) at PassManager.cpp:1323
#19 0x0087464c in llvm::FunctionPassManagerImpl::run (this=0x1606410, F=@0x1603720) at PassManager.cpp:1281
#20 0x008747da in llvm::FunctionPassManager::run (this=0xbffff520, F=@0x1603720) at PassManager.cpp:1226
#21 0x0000352e in main (argc=6, argv=0xbffff5d0) at llc.cpp:317

bugpoint-reduced-simplified.bc (1.53 KB)

Scott,

You're absolutely right. I got very very confused early on and that completely derailed the whole discussion. I'm sorry for being an idiot, I must have had crossed wires.

Now that I actually understand that your goal here is to handle *buildvectors* not "the buildvector argument to a shuffle node", I understand a lot better what you're trying to do (and agree that it has nothing to do with shuffles!).

However, I still prefer a different approach. Are you familiar with the IntrinsicInst class and things like DbgStopPointInst? The basic jist of it is that they are "pseudo classes" that are used with dyn_cast etc that provide very useful helpers.

In the case of LLVM IR and debug stoppoint, a stoppoint is just an llvm call instruction which happens to call the llvm.stoppoint intrinsic. The DbgStopPointInst class "matches" in this case, allowing you to use code like this:

if (DbgStopPointInst *I = dyn_cast< DbgStopPointInst>(someinst)) {
    print(I->getLine());
}

Where getLine() is a very nice helper function.

What do you think about making BuildVectorSDNode work exactly like this? That way, BUILD_VECTOR nodes are just normal SDNodes, but we can still use:

if (BuildVectorSDNode *BV = dyn_cast< BuildVectorSDNode>(N)) { // only works if the opcode is ISD::BUILD_VECTOR
   if (BV->isSplat(info,returned, byref))
      do stuff with info.
}

This makes the BuildVectorSDNode just a nice place to put the various methods for inspecting build vector but doesn't require the uniquing machinery to know about it, and means that things like SplatBits, SplatUndef etc aren't instance variables.

What do you think?

Again, I'm am really really really sorry for the confusion, I did not intend to derail the entire discussion with nonsense :frowning: :frowning:

-Chris

Will look into it. (But all of the PPC tests passed… really, I check that before commiting… honest! :wink:

-scooter

Evan:

I did not encounter this back trace before I committed the newest BuildVectorSDNode patch, which removed all class instance members and passes results back via reference parameters.

-scooter