[llvm] r246371 - New interface function is added to VectorUtils

[James Molloy via llvm-commits]

Hi Elena,

Firstly, it looks like your patch *fixes* a bug: the previous code didn't
check that the InsertVectorElt it found was actually inserting into lane
zero. Could a testcase be added to check this?

Secondly there are some coding style problems here:

+llvm::ConstantDataVector *CV = dyn_cast<llvm::ConstantDataVector>(V);
+if (CV)
+return CV->getSplatValue();

(And several other places): This would be more idiomatically written as:

if (auto *CV = dyn_cast<ConstantDataVector>(V))
  return CV->getSplatValue();

There are other places where you needlessly use the llvm:: namespace prefix
too.

+ // All-zero (our undef) shuffle mask elements.

s/our/or.

for (int i : ShuffleInst->getShuffleMask())

We idiomatically use capitals for our loop induction variables.

if (i != 0 && i != -1)

The function should have a comment stating that it will only detect splats
that insert into the zero'th element of the left-hand vector (idiomatic
splats). The function could easily be made to detect non-idiomatic splats
too, but as long as the function says it explicitly that's good enough, I
think.

llvm::InsertElementInst *InsertEltInst =
    dyn_cast<llvm::InsertElementInst>(ShuffleInst->getOperand(0));

You don't need to use llvm::, and you can use auto to shorten the line.

Cheers,

James

On Sun, 30 Aug 2015 at 11:39 Demikhovsky, Elena via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> I'm sure that it is unrelated. I just added an additional function that
> nobody uses yet.
> How should I check this?
>
> -  Elena
>
> -----Original Message-----
> From: Renato Golin [mailto:renato.golin at linaro.org]
> Sent: Sunday, August 30, 2015 13:07
> To: Demikhovsky, Elena
> Cc: LLVM Commits
> Subject: Re: [llvm] r246371 - New interface function is added to
> VectorUtils
>
> On 30 August 2015 at 08:28, Elena Demikhovsky via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> > Author: delena
> > Date: Sun Aug 30 02:28:18 2015
> > New Revision: 246371
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=246371&view=rev
> > Log:
> > New interface function is added to VectorUtils
>
> Hi Elena,
>
> Some issue with paq8p:
>
> TIMING OUT PROCESS: Output/paq8p.simple
> error: child terminated by signal 9
> TEST Output/paq8p.simple FAILED: process terminated by signal (exit status
> 137)!
>
> I'm also getting segfaults when I run it manually.
>
> http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/106
>
> I've open PR24627 and have reverted in r246379.
>
> We shall investigate this offline.
>
> cheers,
> --renato
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150830/8098e0fe/attachment.html>