<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi Yi,<div><br></div><div>LGTM.</div><div><br></div><div>Please commit directly after you've fixed these typos:</div><div><div>+ // If the type of the truncate is legal, no trucates will be</div><div>+ // introduce in other basic blocks.</div></div><div><br></div><div>=></div><div>s/trucates/truncates/</div><div>s/introduce/introduced/</div><div><br></div><div>Thanks,</div><div>-Quentin</div><div><br></div><div><br><div><div>On Apr 21, 2014, at 10:45 AM, yijiang <<a href="mailto:yjiang@apple.com">yjiang@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Hi Quentin, </div><div><br></div><div>Thank you for the comments. The new patch: </div><div><br></div><div><br></div><div></div></div><span><extractbit-v4-3.patch></span><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div></div><br><div><div>On Apr 21, 2014, at 9:53 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Hi Yi,</div><div><br></div><div>This LGTM with some minor nitpicks:</div><div><br></div><div>#1</div><div><div>+// isExtractBitsCandidateUse - Check if the candidates could</div><div>+// be combined with shift instruction, which includes:</div><div>+// 1. Truncate instruction</div><div>+// 2. And instruction and the imm is a mask of the low bits:</div><div>+// imm & (imm+1) == 0</div></div><div>Make sure your comments use the doxygen style, i.e., replace // by ///.</div><div>At least isExtractBitsCandidateUse and SinkShiftAndTruncate are not using the right format.</div><div><br></div><div><br></div><div>#2</div><div><div>+ // If the use is actually a legal node, there will not be a implicit</div><div>+ // truncate.</div></div><div>s/a implicit/an implicit/</div><div><br></div><div><br></div><div>#3</div><div><div>+ // The type of User is not legal does not necessary introduce</div><div>+ // truncate. We just want to filter most of the cases here for</div><div>+ // compile time sake.</div></div><div>=></div><div>Something along these lines I guess:</div><div>If the type of the truncate is legal, this will not introduce truncates in other basic blocks. Thus, do not traverse its users.</div><div><br></div><div><br></div><div>#4</div><div>+; OPT: if.end</div><div>OPT-LABEL instead?</div><div><br></div><div>+; OPT: if.then17</div><div>Ditto.</div><br><div apple-content-edited="true">
<div style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">-Quentin</div>
</div>
<br><div><div>On Apr 18, 2014, at 5:42 PM, yijiang <<a href="mailto:yjiang@apple.com">yjiang@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Hi Quentin,</div><div><br></div><div>Thank you for your detailed comments! I attached the new patch based on our discussion. Could you help to take a look at it? Thank you!</div><div><br></div></div><span><extractbit-v4-2.patch></span><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Apr 16, 2014, at 5:23 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi Yi,<div><br></div><div>I hope the format of the review will not be too difficult to digest. For the next iterations you may want to create a phabricator (<a href="http://reviews.llvm.org/">http://reviews.llvm.org</a>).</div><div><br></div><div>I have a couple of concerns that I split into two categories:</div><div>- Thing to fix: Are fundamental things that would produce correctness issue or hard to maintain problems.</div><div>- Nice to have: Are nitpick on the code.</div><div><br></div><div>** Things to Fix **</div><div><br></div><div>* Logic *</div><div><br></div><div>#1</div><div>+ (!TLI.isTypeLegal(TLI.getValueType(User->getType())</div><div>This does not seem a sufficient check to guess whether or not a truncate will be issued at legalization.</div><div>We should instead check if the operation is legal for this type.</div><div>Indeed, i16 may not be a legal type, but for instance store i16 may and no truncate will be issued.</div><div><br></div><div>#2</div><div><div>+ TheUse = InsertedShift; /// <———— This does not look right to me.</div><div>+ TruncTheUse = InsertedTrunc;</div><div>+ }</div></div><div>Unless I am mistaken, you are transferring the use, in the original truncate, of the original shift into a use of the new shift in the original truncate.</div><div>This use must be left untouched.</div><div><br></div><div>You may want to check if the original truncate still have uses and nuke it if it does not, though.</div><div><br></div><div> #3</div><div><div>+ assert((Op0->getOpcode() == ISD::TRUNCATE || BiggerPattern ||</div><div>+ (Srl_imm > 0 && Srl_imm < VT.getSizeInBits())) &&</div></div><div><br></div><div>Based on the code this patch change, you shouldn’t have to modify this assert. If you do have to modify it, it seems to me that there is something wrong.</div><div>Please double check.</div><div><br></div><div>#4</div><div><div> // we're looking for a shift of a shift</div><div> uint64_t Shl_imm = 0;</div><div> if (isOpcWithIntImmediate(N->getOperand(0).getNode(), ISD::SHL, Shl_imm)) {</div><div> Opd0 = N->getOperand(0).getOperand(0);</div><div>+ } else if (VT == MVT::i32 && N->getOpcode() == ISD::SRL &&</div><div>+ N->getOperand(0).getNode()->getOpcode() == ISD::TRUNCATE) {</div><div>+ // In this case, 64bit UBFM and 32bit UBFM have the same semantics. Our</div><div>+ // strategy here is to always generate 64bit UBFM. This consistency will</div><div>+ // help the CSE pass later find more redundancy.</div><div>+ Opd0 = N->getOperand(0).getOperand(0);</div><div>+ DstVT = MVT::i64;</div></div><div><br></div><div>There are several problems here.</div><div>A - This part of the code is looking for SHL, not SRL. The part looking for the SRL is later in the same function:</div><div><div> if (!isIntImmediate(N->getOperand(1), Srl_imm))</div><div> return false;</div></div><div>Your logic should be added somewhere here.</div><div>B - You did not update the Srl_imm information. Thus the resulting code may be wrong.</div><div><br></div><div>#5</div><div>+ DstVT = MVT::i64;</div><div>You should reuse VT here (assuming you moved the related code at the right place) like you did in the AND case.</div><div>More importantly, please use the type from the node (and return false on VT != MVT::i64 or at least assert that MVT::i64), not a hard coded one.</div><div><br></div><div>Remove this change:</div><div>- if (VT == MVT::i32)</div><div>+ if (DstVT == MVT::i32)</div><div><br></div><div>#6</div><div><div>+ // If the bit extract operation is 64bit but the original type is 32bit, we</div><div>+ // need to add one EXTRACT_SUBREG.</div><div>+ if (Opc == ARM64::SBFMXri || Opc == ARM64::UBFMXri) {</div><div>+ if (VT == MVT::i32) {</div></div><div>+ SDNode *BFM = CurDAG->getMachineNode(Opc, SDLoc(N), MVT::i64, Ops);</div><div><br></div><div>Technically speaking you should create different Ops when promoting to the i64 type.</div><div>Indeed, current Ops were created with:</div><div><div> SDValue Ops[] = { Opd0, CurDAG->getTargetConstant(LSB, VT),</div><div> CurDAG->getTargetConstant(MSB, VT) };</div></div><div>Thus, the VTs do not match for the constants.</div><div><br></div><div>#7</div><div><div><div>+ // If the bit extract operation is 64bit but the original type is 32bit, we</div><div>+ // need to add one EXTRACT_SUBREG.</div><div>+ if (Opc == ARM64::SBFMXri || Opc == ARM64::UBFMXri) {</div><div>+ if (VT == MVT::i32) {</div></div><div></div></div><div>Sink the second if into the first one. That will reduce the indentation as a bonus :).</div><div><br></div><div>#8</div><div><div>+ if (Opc == ARM64::SBFMXri || Opc == ARM64::UBFMXri) {</div><div>+ if (VT == MVT::i32) {</div></div><div>I am not a big fan of the way you detect the fact that the type has been promoted.</div><div>I think it would be cleaner to add (yet) another reference argument to know whether or not we have to do that.</div><div>Ultimately, it looks like we could benefit from a helper class here, but the refactoring is out of the scope of this patch.</div><div><br></div><div>* Tests *</div><div><br></div><div>#9</div><div>I’d like you add an additional RUN line with opt and check the actual transformation done on the IR.</div><div><br></div><div>#10</div><div>+; CHECK-LABEL: LBB22_2:</div><div>Please do not rely on basic blocks labels. That looks fragile.</div><div>+; CHECK-LABEL: Lloh4:</div><div>This is even more true for these that are added by the ARM64CollectLOH pass.</div><div> </div><div> </div><div>* Comments *</div><div><br></div><div>#11</div><div><div>+/// OptimizeExtractBits - sink the given shift instruction into user blocks if</div><div>+/// the uses could potentially be combined with the shift instruction and</div><div>+/// generate BitExtract instruction. It will only be applied if the architecture</div><div>+/// supports BitExtract instruction.</div><div>+///</div><div>+/// Return true if any changes are made.</div></div><div>You should be more specific here. Like specifying the pattern you are recognizing.</div><div>Indeed the shifts that are supported in this patch are logical and arithmetic right shift.</div><div>In other words, this optimization should help for pattern like this:</div><div>Input:</div><div>XXXXXXPPPXXX</div><div>Output:</div><div>000000000PPP</div><div>(shift right + and of low bits)</div><div><br></div><div>But not for pattern like this:</div><div><div>Input:</div><div>XXXXXXPPPXXX</div><div>Output:</div><div>0000PPP00000</div></div><div>(shift left + and of contiguous bits)</div><div><br></div><div>#12</div><div><div>+ if (UserBB == DefBB) {</div><div>+ // The type of the use is not legal in this architecture so there is a</div><div>+ // implicit truncation.</div><div>+ // for example:</div><div>+ // BB1:</div><div>+ // i64 shift.result = shlr i64 opnd, imm</div><div>+ // trunc.result = trunc shift.result to i16</div><div>+ //</div><div>+ // BB2:</div><div>+ // ----> We will have a implicit truncate here if the architecture does</div><div>+ // not have i16 register.</div></div><div>+ // use i16 trunc.result</div><div><br></div><div>This comment is confusing because:</div><div>A - You mention two basic blocks after a check that both the defBB and the useBB are equal.</div><div>B - The first sentence is missing a ‘if’ I guess.</div><div><br></div><div>What about something like (do not copy/paste, this is not a wonderful wording either :)):</div><div>If UseBB and DefBB are equal and use is a truncate, we may still want to sink both the shift and the truncate within the block of the use of the truncate.</div><div>Indeed, if the use of the truncate is not legal it may produce a truncate at legalization.</div><div><br></div><div><br></div><div>** Nice to Have **</div><div><br></div><div>* lib/CodeGen/CodeGenPrepare.cpp *</div><div><br></div><div>#13</div><div>The body of OptimizeExtractBits gets a bit complex, I think some refactoring (essentially helper functions) would help.</div><div><br></div><div><div>+ // The candidates that could be combined with shift instruction.</div><div>+ // 1. Truncate instruction</div><div>+ // 2. And instruction and the imm is a mask of the low bits: </div><div>+ // imm & (imm+1) == 0</div><div>+ if (!isa<TruncInst>(User)) {</div><div>+ if (User->getOpcode() != Instruction::And ||</div><div>+ !isa<ConstantInt>(User->getOperand(1)))</div><div>+ continue;</div><div>+</div><div>+ unsigned Cimm =</div><div>+ dyn_cast<ConstantInt>(User->getOperand(1))->getZExtValue();</div><div>+</div><div>+ if (Cimm & (Cimm + 1))</div><div>+ continue;</div><div>+ }</div></div><div>=></div><div>move that into a helper function like 'bool isExtractBitsCandidateUse’.</div><div>Then, adapt the loop accordingly.</div><div><br></div><div>#14</div><div>Same for this:</div><div><div>+ // The type of the use is not legal in this architecture so there is a</div><div>+ // implicit truncation.</div><div>+ // for example:</div><div>+ // BB1:</div><div>+ // i64 shift.result = shlr i64 opnd, imm</div><div>+ // trunc.result = trunc shift.result to i16</div><div>+ //</div><div>+ // BB2:</div><div>+ // ----> We will have a implicit truncate here if the architecture does</div><div>+ // not have i16 register.</div><div>+ // use i16 trunc.result</div><div>+ //</div><div>+ if (isa<TruncInst>(User) &&</div><div>+ (TLI.isTypeLegal(TLI.getValueType(ShiftI->getType()))) &&</div><div>+ (!TLI.isTypeLegal(TLI.getValueType(User->getType())))) {</div><div>+ TruncInst *TruncI = dyn_cast<TruncInst>(User);</div><div>+</div><div>+ for (Value::user_iterator TruncUI = TruncI->user_begin(),</div><div>+ TruncE = TruncI->user_end();</div><div>+ TruncUI != TruncE;) {</div><div>+</div><div>+ Use &TruncTheUse = TruncUI.getUse();</div><div>+ Instruction *TruncUser = cast<Instruction>(*TruncUI);</div><div>+ // Preincrement use iterator so we don't invalidate it.</div><div>+</div><div>+ ++TruncUI;</div><div>+</div><div>+ // Don't bother for PHI nodes.</div><div>+ if (isa<PHINode>(TruncUser))</div><div>+ continue;</div><div>+</div><div>+ BasicBlock *TruncUserBB = TruncUser->getParent();</div><div>+</div><div>+ if (UserBB == TruncUserBB)</div><div>+ continue;</div><div>+</div><div>+ BinaryOperator *&InsertedShift = InsertedShifts[TruncUserBB];</div><div>+ CastInst *&InsertedTrunc = InsertedTruncs[TruncUserBB];</div><div>+</div><div>+ if (!InsertedShift && !InsertedTrunc) {</div><div>+ BasicBlock::iterator InsertPt = TruncUserBB->getFirstInsertionPt();</div><div>+ // Sink the shift</div><div>+ if (ShiftI->getOpcode() == Instruction::AShr)</div><div>+ InsertedShift = BinaryOperator::CreateAShr(ShiftI->getOperand(0),</div><div>+ CI, "", InsertPt);</div><div>+ else</div><div>+ InsertedShift = BinaryOperator::CreateLShr(ShiftI->getOperand(0),</div><div>+ CI, "", InsertPt);</div><div>+</div><div>+ // Sink the trunc</div><div>+ BasicBlock::iterator TruncInsertPt =</div><div>+ TruncUserBB->getFirstInsertionPt();</div><div>+ TruncInsertPt++;</div><div>+</div><div>+ InsertedTrunc =</div><div>+ CastInst::Create(TruncI->getOpcode(), InsertedShift,</div><div>+ TruncI->getType(), "", TruncInsertPt);</div><div>+</div><div>+ MadeChange = true;</div><div>+</div><div>+ TheUse = InsertedShift;</div><div>+ TruncTheUse = InsertedTrunc;</div><div>+ }</div><div>+ }</div><div>+ }</div></div><div><br></div><div>You have a big chunk of code to handle a special case. Move that into a helper function.</div><div><br></div><div>* lib/Target/ARM64/ARM64ISelDAGToDAG.cpp *</div><div><br></div><div>#15</div><div><div>+ // If the shift result was truncated, we can still combine them.</div><div>+ Opd0 = Op0->getOperand(0).getNode()->getOperand(0);</div></div><div>You could simply use</div><div>Op0->getOperand(0).getOperand(0)</div><div><br></div><div>Thanks,</div><div><div apple-content-edited="true">
<div style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">-Quentin</div>
</div>
<br><div><div>On Apr 16, 2014, at 9:36 AM, yijiang <<a href="mailto:yjiang@apple.com">yjiang@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Here is the new patch. More comments are appreciated!<br><span><extractbit-v2.patch></span><br>On Apr 16, 2014, at 9:29 AM, Yi Jiang <<a href="mailto:yjiang@apple.com">yjiang@apple.com</a>> wrote:<br><br><blockquote type="cite">You are right. Thank you. <br>On Apr 14, 2014, at 6:59 PM, Matt Arsenault <<a href="mailto:Matthew.Arsenault@amd.com">Matthew.Arsenault@amd.com</a>> wrote:<br><br><blockquote type="cite">On 04/14/2014 05:25 PM, yijiang wrote:<br><blockquote type="cite">+ void setHasExtractBitsInsn(bool hasExtractInsn = false) {<br>+ HasExtractBitsInsn = hasExtractInsn;<br>+ }<br></blockquote>The default argument should be true, since that's what all the others do and since the default value is already false<br></blockquote><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></blockquote><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></blockquote></div><br></div></div></blockquote></div><br></div></blockquote></div><br></div></blockquote></div><br></div></blockquote></div><br></div></body></html>