<div dir="ltr">Hi Sergey,<div><br></div><div>Thanks for your update! For large patch, I personally think it would be a good choice if we can use <a href="http://review.llvm.org">review.llvm.org</a>.</div><div><br></div><div>
Overall I'm OK with the patch. There are some minor nitpicks.</div><div><br></div><div><div>+// Adjusts one cmp instraction to another one if result of adjustment will allow</div><div>+// CSE. Returns true if compare instruction was changed, otherwise false is</div>
<div>+// returned.</div></div><div><br></div><div>s/instraction/instruction/</div><div><br></div><div>+ bool Negative = Opc == AArch64::ADDSWri || Opc == AArch64::ADDSXri;<br></div><div><br></div><div>Can you add comments describing why ADDS implies negative? And I prefer to add brackets for entire right hand expression.</div>
<div><br></div><div><div>+ if (((HeadCmp == AArch64CC::GT && TrueCmp == AArch64CC::LT) ||</div><div>+ (HeadCmp == AArch64CC::LT && TrueCmp == AArch64CC::GT)) &&</div><div>+ std::abs(TrueImm - HeadImm) == 2) {</div>
<div>+ // This branch handles the following cases ({Imm} is the same on both</div><div>+ // sides):</div><div>+ // (a < {Imm} && ...) || (a > {Imm} && ...)</div><div>+ // (a > {Imm} && ...) || (a < {Imm} && ...)</div>
<div><br></div><div>Can you change the comments clarifying at this moment the Machine Instructions are like</div><div><br></div><div><div> // (a > {TrueImm} && ...) || (a < {HeadImm} && ...)</div>
<div> // (a < {TrueImm} && ...) || (a > {HeadImm} && ...)</div><div> //</div></div><div> // and we want to change it to be like,</div><div> //</div><div><div><div> // (a >= {NewImm} && ...) || (a <= {NewImm} && ...)</div>
<div> // (a <= {NewImm} && ...) || (a >= {NewImm} && ...)</div><div><br></div></div></div><div>+ } else if (((HeadCmp == AArch64CC::GT && TrueCmp == AArch64CC::GT) ||<br></div><div>+ (HeadCmp == AArch64CC::LT && TrueCmp == AArch64CC::LT)) &&</div>
<div>+ std::abs(TrueImm - HeadImm) == 1) {</div><div>+ // This branch handles the following cases ({Imm} is the same on both</div><div>+ // sides):</div><div>+ // (a < {Imm} && ...) || (a <= {Imm} && ...)</div>
<div>+ // (a > {Imm} && ...) || (a >= {Imm} && ...)</div><div><br></div></div><div>Can you change the comments clarifying at this moment the Machine Instructions are like</div><div><br></div><div>
<div> // (a > {TrueImm} && ...) || (a > {HeadImm} && ...)<br></div></div><div><div> // (a < {TrueImm} && ...) || (a < {HeadImm} && ...)</div></div><div> //</div><div>
<div> // and we want to change it to be like,</div></div><div> //</div><div><div> // (a <= {NewImm} && ...) || (a > {NewImm} && ...)<br></div><div> // (a < {NewImm} && ...) || (a >= {NewImm} && ...)</div>
</div><div><br></div><div>I know your original comments want to clarify the relationship to the C source code, but it's really hard to understand how it is linked with current Machine Instruction representation. If you still want to describe this, I think the transformation in SelectionDAG part should probably be articulated as well somewhere.</div>
<div><br></div><div>I can't remember why AArch64CC::LE and AArch64CC::GE needn't to be covered at this stage. Do you have answer to that?</div><div><br></div><div>Thanks,<br></div><div>-Jiangning</div><div><br></div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-08-22 2:10 GMT+08:00 Sergey Dmitrouk <span dir="ltr"><<a href="mailto:sdmitrouk@accesssoftek.com" target="_blank">sdmitrouk@accesssoftek.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
Thanks, Jiangning. I made some more changes that make transformations<br>
safer and skips them if there is no potential benefit. I run all test<br>
cases for both apps, which show no regressions.<br>
<br>
The main patch is still named combine-comparisons-by-cse.patch, while<br>
the second file (combine-comparisons-by-cse-changes.diff) contains<br>
only the latest changes for convenience.<br>
<br>
Thanks,<br>
Sergey<br>
<div class=""><br>
On Thu, Aug 14, 2014 at 10:14:31PM -0700, Jiangning Liu wrote:<br>
> Hi Sergey,<br>
</div><div class="">> Thanks for your update!<br>
> Unfortunately, with the latest patch, I can see "miscompare" failures for<br>
> 445.gobmk and 482.sphinx3 in spec2006.<br>
> Can you verify it in your local box?<br>
> I think you can "run" them with qemu following<br>
> <a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-April/072085.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-April/072085.html</a>, if you<br>
> don't really have aarch64 hardware.<br>
> Thanks,<br>
> -Jiangning<br>
><br>
> 2014-08-13 16:51 GMT+08:00 Sergey Dmitrouk <<a href="mailto:sdmitrouk@accesssoftek.com">sdmitrouk@accesssoftek.com</a>>:<br>
><br>
> Hi Jiangning,<br>
> > The 2nd compare instruction should not be under the same condition as<br>
> the<br>
> > 1st one, I think.<br>
><br>
> It should for currently supported transformations, otherwise we can try<br>
</div>> to adjust wrong instruction.A Allowing second compare be followed by<br>
<div class="">> arbitrary instruction is a separate case that needs changes in at least<br>
</div>> two places (need to take into account that we can't touch it).A I can<br>
<div class="">> do<br>
> this if you want to have it now, but it'll increase size of the patch<br>
> even more, so I just added TODO item for it at the top for now.<br>
</div>> >A A Can you try attached small case? With your patch, it may trigger<br>
> >A A segmentation fault.<br>
<div class="">><br>
> I missed handling of "cmp; ...; b.??" in adjustCmp(), it assumed the<br>
</div>> next instruction to be Bcc.A Thank you, I reduced that code and added<br>
> it to tests.A I also added hidden command-line option to enable/disable<br>
<div class="">> the pass to omit recompilation just to disable it.<br>
><br>
> Thanks,<br>
> Sergey<br>
> On Tue, Aug 12, 2014 at 06:20:20PM -0700, Jiangning Liu wrote:<br>
</div>> >A A Hi Sergey,<br>
> >A A The 2nd compare instruction should not be under the same<br>
> condition as the<br>
> >A A 1st one, I think.<br>
> >A A Can you try attached small case? With your patch, it may trigger<br>
> >A A segmentation fault.<br>
<div class="HOEnZb"><div class="h5">> >A A Thanks,<br>
> >A A -Jiangning<br>
</div></div></blockquote></div><br></div>