<html><head><base href="x-msg://122/"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Ana,<div><br></div><div>Thanks for working on this. It does look like an important peephole optimization. Unfortunately I think this patch has some problems.</div><div><br></div><div>1. The instruction selection pattern is looking for a very specific case. Does it still work if the source is not a function argument? For example, if it's the return value of a function call, or a result of a computation, I don't think it will work with this patch. </div><div>2. MOVrs and RSBccri are not needed. You can expand into MOVr with the optional def set to CPSR and a RSBri with the predicate operand filled in.</div><div>3. We want to avoid pseudo instructions that expands into multiple instructions. As you have noticed, this messes up scheduling. Probably the right solution is to expand the instructions at pre-RA scheduling time.</div><div>4. It's a bad idea to have a single instruction that's used for both ARM and Thumb2 mode.</div><div><br></div><div>My suggestion:</div><div>1. ISel should match to an instruction (say ABS, t2ABSri) that's marked with usesCustomInserter = 1.</div><div>2. Add support to lower ABS to MOVr and RSBri with optional def and predicate operands filled in. Please do the same for the Thumb2 variant.</div><div><br></div><div>Bonuses:</div><div>1. Is this sufficient to generate the best code sequence? e.g.</div><div><div> movs r0, r0</div><div> rsbmi r0, r0, #0</div></div><div><br></div><div>This is the best if r0 is a function input. But how about?</div><div> add r0, r1</div><div><div> movs r0, r0</div><div> rsbmi r0, r0, #0</div></div><div><br></div><div>Shouldn't we copy propagate the movs?</div><div><div> adds r0, r1</div><div><div> rsbmi r0, r0, #0</div></div></div><div><br></div><div>One possibility is the scheduler custom expansion code look for the instruction that defines the source and tack the optional def on that instruction. Any other ideas?</div><div><br></div><div>2. How to make the instruction selection code match more cases? My suggestion is to add a new target independent opcode ABS. For targets where this node is legal, i.e. ARM, dag combine can form this instruction rather than the sra + sub sequence. This way, you can write a simple pattern to match the instruction instead of C++ selection code.</div><div><br></div><div>Evan</div><div><br><div><div>On Sep 15, 2011, at 5:32 PM, Ana Pazos wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple"><div class="WordSection1" style="page: WordSection1; "><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Hello,<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">I worked on an LLVM patch to optimize integer ABS idiom for the ARM target and would like to submit it to your review.<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">I experimented with EEMBC benchmarks, in particular MPEG encoding, and noted integer ABS computation happens frequently. Significant speed up was achieved with the optimized idiom for ARM (20% for MPEG encoding).<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><b>Patch details:<o:p></o:p></b></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><b><o:p> </o:p></b></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">LLVM lowers SELECT_CC nodes that represent an integer ABS pattern into ASR/ADD/XOR instructions.<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">It is possible to create an optimized machine idiom for integer ABS on ARM formed by MOVs/RSBmi predicated instructions.<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">This patch modifies ARM-specific files to implement the above optimized machine idiom for integer ABS.<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Generation of the optimized integer ABS idiom is turned on by default. To turn this feature off set -disable-arm-int-abs feature flag.<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">abspatch.diff<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Changes to ARM-specific files to implement optimized integer ABS idiom.<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">abstestpatch.diff<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">ARM/iabs.ll and Thumb/iabs.ll tests check for the non-optimized integer ABS idiom (ASR/ADD/XOR).<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">When applying abspatch.diff these tests fail. So patched the tests to set -disable-arm-int-abs flag to prevent the compiler from generating optimized integer ABS pattern and allow the non-optimized idiom to be checked.<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">iabsopt.ll<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Similar to ARM/iabs.ll and Thumb/iabs.ll tests except that it checks for the optimized integer ABS idiom and checks for all possible test conditions.<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">failures.txt<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Failure report and explanation from running llvm/test and projects/test-suite on ARM.<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">I noted failures running llvm/test (svn version 139318) and projects/test-suite (svn revision 139319) on ARM. Are these failures expected?<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Thank you,<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Ana.<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div></div><span><abspatch.diff></span><span><abstestpatch.diff></span><span><failures.txt></span><span><iabsopt.ll></span>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" style="color: blue; text-decoration: underline; ">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" style="color: blue; text-decoration: underline; ">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></div></blockquote></div><br></div></body></html>