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

[Nick Lewycky]

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.

Nick
-------------- 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>