<div dir="ltr">Hi Elena,<div><br></div><div>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?</div><div><br></div><div>Secondly there are some coding style problems here:</div><div><br></div><div><div>+llvm::ConstantDataVector *CV = dyn_cast<llvm::ConstantDataVector>(V);</div><div><span style="line-height:1.5;font-size:13.1999998092651px">+</span><span style="line-height:1.5;font-size:13.1999998092651px">if (CV)</span></div><div>+return CV->getSplatValue();</div></div><div><br></div><div>(And several other places): This would be more idiomatically written as:</div><div><br></div><div>if (auto *CV = dyn_cast<ConstantDataVector>(V))</div><div> return CV->getSplatValue();</div><div><br></div><div>There are other places where you needlessly use the llvm:: namespace prefix too.</div><div><br></div><div><span style="color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:10px;line-height:16px;white-space:pre-wrap;background-color:rgb(170,255,170)">+ // All-zero (our undef) shuffle mask elements.</span><br></div><div><span style="color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:10px;line-height:16px;white-space:pre-wrap;background-color:rgb(170,255,170)"><br></span></div><div>s/our/or.</div><div><br></div><div><span style="color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:10px;line-height:16px;white-space:pre-wrap;background-color:rgb(170,255,170)">for (int i : ShuffleInst->getShuffleMask())</span><br></div><div><span style="color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:10px;line-height:16px;white-space:pre-wrap;background-color:rgb(170,255,170)"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">We idiomatically use capitals for our loop induction variables.</span><br></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:10px;line-height:16px;white-space:pre-wrap;background-color:rgb(170,255,170)">if (i != 0 && i != -1)</span><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:10px;line-height:16px;white-space:pre-wrap;background-color:rgb(170,255,170)"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">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.</span><span style="color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:10px;line-height:16px;white-space:pre-wrap;background-color:rgb(170,255,170)"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><div>llvm::InsertElementInst *InsertEltInst =</div><div> dyn_cast<llvm::InsertElementInst>(ShuffleInst->getOperand(0));</div></div><div><br></div><div>You don't need to use llvm::, and you can use auto to shorten the line.</div><div><br></div><div>Cheers,</div><div><br></div><div>James</div></div><br><div class="gmail_quote"><div dir="ltr">On Sun, 30 Aug 2015 at 11:39 Demikhovsky, Elena via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I'm sure that it is unrelated. I just added an additional function that nobody uses yet.<br>
How should I check this?<br>
<br>
- Elena<br>
<br>
-----Original Message-----<br>
From: Renato Golin [mailto:<a href="mailto:renato.golin@linaro.org" target="_blank">renato.golin@linaro.org</a>]<br>
Sent: Sunday, August 30, 2015 13:07<br>
To: Demikhovsky, Elena<br>
Cc: LLVM Commits<br>
Subject: Re: [llvm] r246371 - New interface function is added to VectorUtils<br>
<br>
On 30 August 2015 at 08:28, Elena Demikhovsky via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: delena<br>
> Date: Sun Aug 30 02:28:18 2015<br>
> New Revision: 246371<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=246371&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=246371&view=rev</a><br>
> Log:<br>
> New interface function is added to VectorUtils<br>
<br>
Hi Elena,<br>
<br>
Some issue with paq8p:<br>
<br>
TIMING OUT PROCESS: Output/paq8p.simple<br>
error: child terminated by signal 9<br>
TEST Output/paq8p.simple FAILED: process terminated by signal (exit status 137)!<br>
<br>
I'm also getting segfaults when I run it manually.<br>
<br>
<a href="http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/106" rel="noreferrer" target="_blank">http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/106</a><br>
<br>
I've open PR24627 and have reverted in r246379.<br>
<br>
We shall investigate this offline.<br>
<br>
cheers,<br>
--renato<br>
---------------------------------------------------------------------<br>
Intel Israel (74) Limited<br>
<br>
This e-mail and any attachments may contain confidential material for<br>
the sole use of the intended recipient(s). Any review or distribution<br>
by others is strictly prohibited. If you are not the intended<br>
recipient, please contact the sender and delete all copies.<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>