<div dir="ltr">Hi Sergey,<div><br></div><div>I looked at your code again, and I believe this piece of code is incorrect,</div><div><br></div><div><div>+ // Change immediate in comparison instruction (ADDS or SUBS).</div><div>
+ MachineInstr *MI = BuildMI(*MBB, CmpMI, CmpMI->getDebugLoc(), TII->get(Opc))</div><div>+ .addOperand(CmpMI->getOperand(0))</div><div>+ .addOperand(CmpMI->getOperand(1))</div><div>+ .addImm(imm + correction)</div>
<div>+ .addOperand(CmpMI->getOperand(3));</div><div>+ CmpMI->eraseFromParent();</div><div>+</div><div>+ MachineInstr *BrMI = MI->getNextNode();</div><div>+</div><div>+ // Change condition in branch instruction.</div>
<div>+ BuildMI(*MBB, BrMI, BrMI->getDebugLoc(), TII->get(AArch64::Bcc))</div><div>+ .addImm(getAdjustedCmp(Cmp))</div><div>+ .addOperand(BrMI->getOperand(1));</div><div>+ BrMI->eraseFromParent();</div>
</div><div><br></div><div>Here BrMI may not really be a branch instruction. You need to check this before entering adjustCmp.</div><div><br></div><div>Thanks,</div><div>-Jiangning</div><div><br></div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">2014-08-12 8:04 GMT+08:00 Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="">----- Original Message -----<br>
> From: "Sergey Dmitrouk" <<a href="mailto:sdmitrouk@accesssoftek.com">sdmitrouk@accesssoftek.com</a>><br>
> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
> Cc: "<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a> for LLVM" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>>, "Jiangning Liu" <<a href="mailto:liujiangning1@gmail.com">liujiangning1@gmail.com</a>><br>
</div><div class="">> Sent: Monday, August 11, 2014 7:31:47 AM<br>
> Subject: Re: [PATCH][AArch64] Enable sign check optimization by CSE<br>
><br>
</div><div class="">> Thanks, changed that part to:<br>
><br>
> Due to the way selection DAG part that corresponds to switch case<br>
> is built,<br>
<br>
</div>I recommend something like this: "As a result of the canonicalization employed by SelectionDAGBuilder::visitSwitchCase, DAGCombine, and other target-specific code, "<br>
<div class=""><br>
><br>
> Jiangning gave a nice overview of what really happens here:<br>
><br>
> <a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140721/227116.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140721/227116.html</a><br>
><br>
> I'm not sure whether CodeGenPrep is able to perform transformations<br>
> like<br>
> this one.<br>
<br>
</div>Probably not. Based on Jiangning's explanation, the SDAG builder's canonicalization, and later code, is at fault here.<br>
<span class="HOEnZb"><font color="#888888"><br>
-Hal<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> Cheers,<br>
> Sergey<br>
><br>
> On Mon, Aug 11, 2014 at 04:56:25AM -0700, Hal Finkel wrote:<br>
> > ----- Original Message -----<br>
> > > From: "Sergey Dmitrouk" <<a href="mailto:sdmitrouk@accesssoftek.com">sdmitrouk@accesssoftek.com</a>><br>
> > > To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
> > > Cc: "<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a> for LLVM"<br>
> > > <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>>, "Jiangning Liu"<br>
> > > <<a href="mailto:liujiangning1@gmail.com">liujiangning1@gmail.com</a>><br>
> > > Sent: Monday, August 11, 2014 3:29:34 AM<br>
> > > Subject: Re: [PATCH][AArch64] Enable sign check optimization by<br>
> > > CSE<br>
> > ><br>
> > > Hi Hal,<br>
> > ><br>
> > > thanks for your comments. getNexeNode() works. I added more<br>
> > > verbose<br>
> > > comment at the top and examples in C after conditions. I didn't<br>
> > > provide<br>
> > > more detailed examples for the later as it would repeat the top<br>
> > > comment.<br>
> > > Changes of instructions are quite straightforward once you get<br>
> > > the<br>
> > > idea<br>
> > > that value is changed by one and comparison inclusive/exclusive<br>
> > > type<br>
> > > is<br>
> > > switched. Still can add it if you prefer, just don't want to<br>
> > > repeat<br>
> > > same information in several places.<br>
> ><br>
> > Thanks, these comments are much better.<br>
> ><br>
> > +// Unfortunately, due to they way other parts implemented,<br>
> > corresponding<br>
> ><br>
> > parts implemented -> parts are implemented<br>
> ><br>
> > Also, what other parts? Specifically, does this happen at the<br>
> > SelectionDAG stage or later? If not, I think this logic would be<br>
> > better in CodeGenPrep instead of here.<br>
> ><br>
> > -Hal<br>
> ><br>
> > ><br>
> > > Updated patch is attached.<br>
> > ><br>
> > > Regards,<br>
> > > Sergey<br>
> > ><br>
> > > On Sun, Aug 10, 2014 at 05:32:57PM -0700, Hal Finkel wrote:<br>
> > > > Hi Sergey,<br>
> > > ><br>
> > > > + MachineInstr *BrMI =<br>
> > > > &*std::next(MachineBasicBlock::iterator(MI));<br>
> > > ><br>
> > > > I think this can be: MachineInstr *BrMI = MI->getNextNode();<br>
> > > ><br>
> > > > + if (((HeadCmp == AArch64CC::GT && TrueCmp ==<br>
> > > > AArch64CC::LT) ||<br>
> > > > + (HeadCmp == AArch64CC::LT && TrueCmp ==<br>
> > > > AArch64CC::GT))<br>
> > > > &&<br>
> > > > + std::abs(TrueImm - HeadImm) == 2) {<br>
> > > > + adjustCmp(HeadCmpMI, HeadCmp);<br>
> > > > + adjustCmp(TrueCmpMI, TrueCmp);<br>
> > > > + Changed = true;<br>
> > > > + } else if (((HeadCmp == AArch64CC::GT && TrueCmp ==<br>
> > > > AArch64CC::GT) ||<br>
> > > > + (HeadCmp == AArch64CC::LT && TrueCmp ==<br>
> > > > AArch64CC::LT)) &&<br>
> > > > + std::abs(TrueImm - HeadImm) == 1) {<br>
> > > > + if (HeadImm < TrueCmp) {<br>
> > > > + adjustCmp(HeadCmpMI, HeadCmp);<br>
> > > > + } else {<br>
> > > > + adjustCmp(TrueCmpMI, TrueCmp);<br>
> > > > + }<br>
> > > > + Changed = true;<br>
> > > > + }<br>
> > > > + }<br>
> > > ><br>
> > > > I think some comments here would be helpful here giving an<br>
> > > > example<br>
> > > > (before and after) for each of these cases. You provide an<br>
> > > > example<br>
> > > > in C form in your e-mail below, but no such example appears in<br>
> > > > the<br>
> > > > comments in the patch (and relating the example at the<br>
> > > > instruction<br>
> > > > level would be good too).<br>
> > > ><br>
> > > > Likewise, the comment at the top of the file should do at least<br>
> > > > as<br>
> > > > good a job at explaining what the pass does as your e-mail.<br>
> > > ><br>
> > > > Thanks,<br>
> > > > Hal<br>
> > > ><br>
> > > > ----- Original Message -----<br>
> > > > > From: "Sergey Dmitrouk" <<a href="mailto:sdmitrouk@accesssoftek.com">sdmitrouk@accesssoftek.com</a>><br>
> > > > > To: "Jiangning Liu" <<a href="mailto:liujiangning1@gmail.com">liujiangning1@gmail.com</a>><br>
> > > > > Cc: "<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a> for LLVM"<br>
> > > > > <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>><br>
> > > > > Sent: Sunday, August 10, 2014 1:25:38 PM<br>
> > > > > Subject: Re: [PATCH][AArch64] Enable sign check optimization<br>
> > > > > by<br>
> > > > > CSE<br>
> > > > ><br>
> > > > > Hi Jiangning,<br>
> > > > ><br>
> > > > > Please find attached patch that adds new pass. It analyzes<br>
> > > > > branches<br>
> > > > > and<br>
> > > > > adjusts comparisons and conditions to make compare<br>
> > > > > instructions<br>
> > > > > look<br>
> > > > > the<br>
> > > > > same, so that CSE pass could remove them. The patch is big,<br>
> > > > > but<br>
> > > > > half<br>
> > > > > of<br>
> > > > > it is test file (tried to make it simpler, but it leads to<br>
> > > > > generating<br>
> > > > > different assembly) and another half is the new pass, which<br>
> > > > > is<br>
> > > > > hard<br>
> > > > > to<br>
> > > > > break into smaller patches. I think it demonstrates general<br>
> > > > > idea (comments in test has comments in C): for code like<br>
> > > > ><br>
> > > > > if ((a > 5 && b == c) || (a >= 5 && b == d)) {<br>
> > > > ><br>
> > > > > a is compared with 5 only once with the patch.<br>
> > > > ><br>
> > > > > Currently only SUBS and ADDS followed by b.?? are supported.<br>
> > > > > If<br>
> > > > > this<br>
> > > > > patch is accepted, it should be possible to additionally:<br>
> > > > > * cover TBNZ/TBZ, which are now used for checks like (x < 0)<br>
> > > > > and<br>
> > > > > doesn't let my patch optimize this case; does it make<br>
> > > > > sense to<br>
> > > > > use<br>
> > > > > one CMP instruction instead of TBNZ followed by CMP?<br>
> > > > > * handle other conditional instructions (e.g. CSET).<br>
> > > > ><br>
> > > > > Comments are welcome, not sure I did it all correctly. Maybe<br>
> > > > > need to<br>
> > > > > add command-line option for the pass, TODO comments, debug<br>
> > > > > output,<br>
> > > > > statistics or rewrite some parts completely.<br>
> > > > ><br>
> > > > > Cheers,<br>
> > > > > Sergey<br>
> > > > ><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" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> > > > ><br>
> > > ><br>
> > > > --<br>
> > > > Hal Finkel<br>
> > > > Assistant Computational Scientist<br>
> > > > Leadership Computing Facility<br>
> > > > Argonne National Laboratory<br>
> > ><br>
> ><br>
> > --<br>
> > Hal Finkel<br>
> > Assistant Computational Scientist<br>
> > Leadership Computing Facility<br>
> > Argonne National Laboratory<br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div></div></blockquote></div><br></div>