<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">
Hi Michael,
<div class=""><br class="">
</div>
<div class="">I’ve addressed as many of these as possible in r252469.</div>
<div class=""><br class="">
</div>
<div class="">Cheers,</div>
<div class=""><br class="">
</div>
<div class="">James</div>
<div class=""><br class="">
<div>
<blockquote type="cite" class="">
<div class="">On 6 Nov 2015, at 17:38, Michael Zolotukhin <<a href="mailto:mzolotukhin@apple.com" class="">mzolotukhin@apple.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Hi
James,</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
On Nov 6, 2015, at 7:16 AM, James Molloy <<a href="mailto:james.molloy@arm.com" class="">james.molloy@arm.com</a>> wrote:<br class="">
<br class="">
Hi Michael,<br class="">
<br class="">
I’ll get around to implementing your comments on Monday, if that’s OK?<br class="">
</blockquote>
<span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Sure!</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<br class="">
<blockquote type="cite" class="">Should we have the same issue in the SLP vectorizer as well?<br class="">
</blockquote>
<br class="">
Yes; that’s why I made this a utility and decoupled it from the loop vectoriser.<br class="">
</blockquote>
<span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Cool,
thanks!</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<br class="">
<blockquote type="cite" class="">Would it be better to return the map by reference to avoid unnecessary copying<br class="">
</blockquote>
<br class="">
Silviu mentioned this in the review. I believe this should be move-constructed as we’re returning a temporary with a move operator, so zero cost.<br class="">
</blockquote>
<span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Got
it.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<br class="">
<blockquote type="cite" class="">Why can’t we just use Instruction* instead of Value* in all these sets? It looks like it could save some casts in the code<br class="">
</blockquote>
<br class="">
SGTM, I’ll do this.<br class="">
<br class="">
<blockquote type="cite" class="">Unneeded whitespace. There are also some other spots formatted badly - maybe reformat it while the code is still relatively fresh?<br class="">
</blockquote>
<br class="">
Will do.<br class="">
<br class="">
<blockquote type="cite" class="">We look for I->getOperand(0) in MinBWs, and then use MinBWs[I] (not operand 0) - is it intentional? If not, we can also avoid double querying MinBWs by using MinBWs.find.<br class="">
</blockquote>
<br class="">
Sounds reasonable to me. Will do.<br class="">
<br class="">
<blockquote type="cite" class="">Do we really need to run all these passes to check loop-vectorizer?<br class="">
</blockquote>
<br class="">
We do need instcombine as we rely on it to sort out our mess. I’ll take a look at paring down the pass list and see what we actually need. I agree it does look a bit overkill at the moment…<br class="">
</blockquote>
<br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Thanks
for doing this!</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Michael</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<br class="">
Cheers,<br class="">
<br class="">
James<br class="">
<br class="">
<blockquote type="cite" class="">On 6 Nov 2015, at 08:13, Michael Zolotukhin <<a href="mailto:mzolotukhin@apple.com" class="">mzolotukhin@apple.com</a>> wrote:<br class="">
<br class="">
Hi James,<br class="">
<br class="">
Thanks for working on this! Should we have the same issue in the SLP vectorizer as well?<br class="">
<br class="">
Also, some remarks inline:<br class="">
<br class="">
<blockquote type="cite" class="">@@ -434,3 +437,130 @@ llvm::Value *llvm::getSplatValue(Value *<br class="">
<br class="">
return InsertEltInst->getOperand(1);<br class="">
}<br class="">
+<br class="">
+DenseMap<Instruction*, uint64_t> llvm::computeMinimumValueSizes(<br class="">
+ ArrayRef<BasicBlock*> Blocks, DemandedBits &DB,<br class="">
+ const TargetTransformInfo *TTI) {<br class="">
</blockquote>
Would it be better to return the map by reference to avoid unnecessary copying?<br class="">
<br class="">
<blockquote type="cite" class="">+<br class="">
+ // DemandedBits will give us every value's live-out bits. But we want<br class="">
+ // to ensure no extra casts would need to be inserted, so every DAG<br class="">
+ // of connected values must have the same minimum bitwidth.<br class="">
+ EquivalenceClasses<Value*> ECs;<br class="">
+ SmallVector<Value*,16> Worklist;<br class="">
+ SmallPtrSet<Value*,4> Roots;<br class="">
+ SmallPtrSet<Value*,16> Visited;<br class="">
+ DenseMap<Value*,uint64_t> DBits;<br class="">
</blockquote>
Why can’t we just use Instruction* instead of Value* in all these sets? It looks like it could save some casts in the code.<br class="">
<br class="">
<blockquote type="cite" class="">@@ -2009,6 +2034,7 @@ InnerLoopVectorizer::getVectorValue(Valu<br class="">
// If this scalar is unknown, assume that it is a constant or that it is<br class="">
// loop invariant. Broadcast V and save the value for future uses.<br class="">
Value *B = getBroadcastInstrs(V);<br class="">
+<br class="">
</blockquote>
Unneeded whitespace. There are also some other spots formatted badly - maybe reformat it while the code is still relatively fresh?<br class="">
<br class="">
<blockquote type="cite" class="">@@ -5168,6 +5314,8 @@ LoopVectorizationCostModel::getInstructi<br class="">
case Instruction::ICmp:<br class="">
case Instruction::FCmp: {<br class="">
Type *ValTy = I->getOperand(0)->getType();<br class="">
+ if (VF > 1 && MinBWs.count(dyn_cast<Instruction>(I->getOperand(0))))<br class="">
+ ValTy = IntegerType::get(ValTy->getContext(), MinBWs[I]);<br class="">
</blockquote>
We look for I->getOperand(0) in MinBWs, and then use MinBWs[I] (not operand 0) - is it intentional? If not, we can also avoid double querying MinBWs by using MinBWs.find.<br class="">
<br class="">
<blockquote type="cite" class="">--- llvm/trunk/test/Transforms/LoopVectorize/AArch64/loop-vectorization-factors.ll (added)<br class="">
+++ llvm/trunk/test/Transforms/LoopVectorize/AArch64/loop-vectorization-factors.ll Mon Oct 12 07:34:45 2015<br class="">
@@ -0,0 +1,243 @@<br class="">
+; RUN: opt -S < %s -basicaa -loop-vectorize -simplifycfg -instsimplify -instcombine -licm -force-vector-interleave=1 2>&1 | FileCheck %s<br class="">
</blockquote>
Do we really need to run all these passes to check loop-vectorizer?<br class="">
<br class="">
<br class="">
Thanks,<br class="">
Michael<br class="">
<br class="">
</blockquote>
<br class="">
<br class="">
________________________________<br class="">
<br class="">
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any
purpose, or store or copy the information in any medium. Thank you.</blockquote>
</div>
</blockquote>
</div>
<br class="">
</div>
<br>
<hr>
<font face="Arial" color="Black" size="2"><br>
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any
purpose, or store or copy the information in any medium. Thank you.<br>
</font>
</body>
</html>