<div dir="ltr">Hi Michael,<br><br><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">> For instance, functions like `isReInterleaveMask` look totally target independent. Would it be possible to make a target-independent implementation with a bunch of target hooks (like `getInterleavedStoreIntrinsic(</span><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">Factor, Size)`)? Then, ARM32 and ARM64 patches would differ only in those hooks, while the rest of the code would be reused.</span><br style="font-size:13.1999998092651px;line-height:19.7999992370605px"></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">Chatting with Hao yesterday, The problem is not how to expose reuse, but where in-tree to put the common, reusable parts. There's no obvious place to hang it off at the moment.</span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">Renato's suggested lib/CodeGen - is that a reasonable place?</span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">I LGTM'd because I didn't see this problem being solved in a short time; I'd have held off approving the ARM32 one until it was sorted though.</span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">Cheers,</span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">James</span></div><br><div class="gmail_quote"><div dir="ltr">On Thu, 11 Jun 2015 at 21:37 Michael Zolotukhin <<a href="mailto:mzolotukhin@apple.com">mzolotukhin@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Hao,<br>
<br>
Thanks for the patch. The general idea sounds good to me, but I have some questions regarding some implementation details. To address them, it would actually be helpful to see the ARM32 patch too, since I expect that a lot of the stuff can be reused, as James and Renato have already mentioned.<br>
<br>
For instance, functions like `isReInterleaveMask` look totally target independent. Would it be possible to make a target-independent implementation with a bunch of target hooks (like `getInterleavedStoreIntrinsic(Factor, Size)`)? Then, ARM32 and ARM64 patches would differ only in those hooks, while the rest of the code would be reused.<br>
<br>
<br>
================<br>
Comment at: lib/Target/AArch64/AArch64InterleavedAccess.cpp:146-148<br>
@@ +145,5 @@<br>
+ unsigned &Index) {<br>
+ unsigned NumElts = Mask.size();<br>
+ if (NumElts < 2)<br>
+ return false;<br>
+<br>
----------------<br>
```<br>
if (Mask.size() < 2)<br>
return false;<br>
```<br>
?<br>
<br>
================<br>
Comment at: lib/Target/AArch64/AArch64InterleavedAccess.cpp:247<br>
@@ +246,3 @@<br>
+<br>
+ unsigned Index;<br>
+ if (!isDeInterleaveMaskOfFactor(Shuffles[i]->getShuffleMask(), Factor,<br>
----------------<br>
Is there a need to redeclare `Index` here?<br>
<br>
================<br>
Comment at: lib/Target/AArch64/AArch64InterleavedAccess.cpp:279<br>
@@ +278,3 @@<br>
+ ShuffleVectorInst *SV = Shuffles[i];<br>
+ unsigned Index = Indices[i];<br>
+<br>
----------------<br>
One more redeclaration of `Index`.<br>
<br>
================<br>
Comment at: lib/Target/AArch64/AArch64InterleavedAccess.cpp:351<br>
@@ +350,3 @@<br>
+<br>
+ SmallVector<Value *, 5> Ops;<br>
+<br>
----------------<br>
Why `5`?<br>
<br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10335&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=R5WOgx5CVxOPGrYL3oCXz52WWqTmAoz6W8Nos-54MPc&s=l_8nMTpzmk1mHZdjd8TeJUNFzG2m61OhK808uyE6Kek&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10335</a><br>
<br>
EMAIL PREFERENCES<br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=R5WOgx5CVxOPGrYL3oCXz52WWqTmAoz6W8Nos-54MPc&s=N0-8eQtFKN3w2_BjjY9H8u0jNkXmLnbBK6o5ZjqRBsw&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>