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

[Demikhovsky, Elena via llvm-commits]

Hi James,

I decided to put the style changes under review to avoid multiple commit iterations.
I’ll add a test case in the next commit, where I’m going to use this new interface.

Thanks.

-           Elena

From: James Molloy [mailto:james at jamesmolloy.co.uk]
Sent: Sunday, August 30, 2015 13:48
To: Demikhovsky, Elena; Renato Golin
Cc: LLVM Commits
Subject: Re: [llvm] r246371 - New interface function is added to VectorUtils

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<mailto: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<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<mailto: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<mailto:llvm-commits at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
---------------------------------------------------------------------
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150830/961f6e2a/attachment.html>