[llvm-commits] PATCH: new Value::getThruSplat API

Nick Lewycky nlewycky at google.com
Fri Oct 29 19:28:13 PDT 2010

On 27 October 2010 17:42, Dan Gohman <gohman at apple.com> wrote:

> On Oct 27, 2010, at 4:53 PM, Nick Lewycky wrote:
> > On 27 October 2010 10:57, Dan Gohman <gohman at apple.com> wrote:
> >
> > On Oct 26, 2010, at 11:34 PM, Nick Lewycky wrote:
> >
> > > This patch adds a new method on Value called getThruSplat() which
> returns the splatted value iff this is a ConstantValue with a splat, or
> 'this' otherwise. I demonstrate its usage by updating InstructionSimplify to
> uniformly handle splat vectors alongside ConstantInt values.
> >
> > Looks reasonable to me.
> >
> > How about "getScalarValue" instead of "getThruSplat"?
> > I don't have strong opinions about it though.
> >
> > ConstantArray and ConstantStruct aren't scalar and this method could
> return those. For that matter, it could even return a vector, constant or
> not.
> >
> > My concern is to make very sure that this is the API we really want. Are
> we sure that "dyn_cast<ConstantInt>(V->getThruSplat())" doesn't do any more
> work than the minimum (or at least, that the compiler can't optimize away)?
> And is it okay to put this work on the lookup side? We will do this test
> more often this way than if we did the work at the point of creation, which
> is the pattern we follow in the rest of LLVM.
> With the current class hierarchy, that dyn_cast really is needed. The splat
> value
> could theoretically be some crazy unfoldable ConstantExpr.

Sure, but if the input is a ConstantStruct for example, will its value ID be
loaded and tested twice? I'm relying on jump threading which both GCC and
LLVM ought to be capable of (nowadays; I'm sure llvm didn't use to be able
to do it).

It might be nice to
> push it into the API though, and have
>  ConstantInt *getAsConstantInt();
>  ConstantFP *getAsConstantFP();
> instead though, if you're going to be doing this a lot.

I'm not a fan, it sounds as though you could get any Value as a
ConstantInt/FP which just doesn't make sense.

> The most radical alternative API would be to factor out a common base that
> represents a ConstantInt+ConstantVector(with splat) and another for
> ConstantFP+ConstantVector(with splat), then teach ConstantVector to return
> such a splatted CV from its getter. Sadly if you follow this through to its
> conclusion, you end up using multiple inheritance in the Constant hierarchy
> which causes all sorts of forms of pain.
> I agree, don't do MI. But I think if you wanted to take this approach, you
> could
> make ConstantInt and ConstantFP capable of having vector types, but not be
> subclasses of ConstantVector. ConstantVector would just be for cases where
> it's
> really necessary to have an array of operands. This sounds like a big and
> scary
> change, though there are some reasons it might not have to be.
> With this approach, vector ConstantAggregateZero could go away.

Doing that would force me to audit every single use of ConstantInt and
ConstantFP to make sure they're okay with splat vectors instead of real
int's/fp's, and adding tests on the type if necessary. Do you think that
would be a better API at the end of the day? My proposed patch would be more
of an opt-in approach.

Which reminds me; your current patch doesn't handle vector
> ConstantAggregateZero.

Good catch! Updated patch attached.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101029/46fa813f/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: get-thru-splat-2.patch
Type: text/x-patch
Size: 3496 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101029/46fa813f/attachment.bin>

More information about the llvm-commits mailing list