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

[Nick Lewycky]

On 29 October 2010 21:34, Chris Lattner <clattner at apple.com> wrote:

> On Oct 29, 2010, at 7:28 PM, Nick Lewycky wrote:
>
> Which reminds me; your current patch doesn't handle vector
>> ConstantAggregateZero.
>>
>
> Good catch! Updated patch attached.
>
>
> Some random comments:
>
> +  /// getThruSplat - This method returns the singular splat value held by
> this
> +  /// value if it is a ConstantVector holding a splat, otherwise it
> returns
> +  /// itself.
>
> This description is confusing to me.  How about something like:
>
> If the current value is a constant vector where all of the elements have
> the same value, return that value.  Otherwise, return the current value.
>

Much better. Thanks!


> Also, the way it is used is confusing.  Why not name it "getSplatElement"
> and have it return null on failure?
>

Because I want to preserve the pattern of a declaring a single variable
inside an if-statement with a dyn_cast. I'm planning to find and update code
which currently does that with a test for ConstantInt/FP and don't want to
add lots of this pattern:

  Value *V = value;
  if (ConstantVector *CV = dyn_cast<ConstantVector>(V))
    if (Constant *Splat = CV->getSplatValue())
      V = Splat;
  if (ConstantInt *CI = dyn_cast<ConstantInt>(V)) {
    ...

all over. If getSplatElement() returned NULL instead of "this" then I would
only be able to remove one line of that (the test for ConstantVector).

I guess I don't really understand what makes the replacement code confusing.

+Value *Value::getThruSplat() {
> +  switch (getValueID()) {
>
> This would be more natural and elegant written with two dyncasts instead of
> a switch.  The premature optimization isn't worth it, and llvm at least will
> turn the two dyncasts into a switch anyway.
>

Okay!

Nick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101029/455e1802/attachment.html>