Thanks very much, Renato! I looked into the regressions, there're many cases have similar context as the case I mentioned but N just has only one use. In this case, the canonicalisation won't make it more profitable, will only prevent the possible folding or make the sequence is not expected. The regressions be eliminated, after I limit the expanding to not "N->hasOneUse()". And now only one regression left: FAIL: LLVM :: CodeGen/Thumb2/machine-licm.ll (18865 of 29222) ******************** TEST 'LLVM :: CodeGen/Thumb2/machine-licm.ll' FAILED ******************** Script: -- /home/likewise-open/SPREADTRUM/ jojo.ma/jojoma/source/llvm/llvm-linaro/llvm/build/./bin/llc < /home/likewise-open/SPREADTRUM/ jojo.ma/jojoma/source/llvm/llvm-linaro/llvm/llvm/test/CodeGen/Thumb2/machine-licm.ll -mtriple=thumbv7-apple-darwin -mcpu=cortex-a8 -relocation-model=dynamic-no-pic -disable-fp-elim | /home/likewise-open/SPREADTRUM/ jojo.ma/jojoma/source/llvm/llvm-linaro/llvm/build/./bin/FileCheck /home/likewise-open/SPREADTRUM/ jojo.ma/jojoma/source/llvm/llvm-linaro/llvm/llvm/test/CodeGen/Thumb2/machine-licm.ll /home/likewise-open/SPREADTRUM/ jojo.ma/jojoma/source/llvm/llvm-linaro/llvm/build/./bin/llc < /home/likewise-open/SPREADTRUM/ jojo.ma/jojoma/source/llvm/llvm-linaro/llvm/llvm/test/CodeGen/Thumb2/machine-licm.ll -mtriple=thumbv7-apple-darwin -mcpu=cortex-a8 -relocation-model=pic -disable-fp-elim | /home/likewise-open/SPREADTRUM/ jojo.ma/jojoma/source/llvm/llvm-linaro/llvm/build/./bin/FileCheck /home/likewise-open/SPREADTRUM/ jojo.ma/jojoma/source/llvm/llvm-linaro/llvm/llvm/test/CodeGen/Thumb2/machine-licm.ll --check-prefix=PIC -- Exit Code: 1 Command Output (stderr): -- /home/likewise-open/SPREADTRUM/ jojo.ma/jojoma/source/llvm/llvm-linaro/llvm/llvm/test/CodeGen/Thumb2/machine-licm.ll:88:10: error: expected string not found in input ; CHECK: movw {{(r[0-9])|(lr)}}, #32768 ^ <stdin>:56:2: note: scanning from here movw r12, #32768 ^ The outputs before and after the canonicalisation are: - before canonicalisation _t3: @ BB#0: @ %bb.nph push {r7, lr} mov r7, sp movw lr, #32768 movs r2, #0 movw r9, #16386 movw r12, #65534 movt lr, #65535 LBB2_1: @ %bb @ =>This Inner Loop Header: Depth=1 eor.w r3, r0, r1 adds r2, #1 ands r3, r3, #1 it ne eorne.w r1, r1, r9 and.w r3, r1, r12 orr.w r1, lr, r3, lsr #1 it eq lsreq r1, r3, #1 ubfx r0, r0, #1, #7 uxtb r3, r2 cmp r3, #8 bne LBB2_1 @ BB#2: @ %bb8 uxth r0, r1 pop {r7, pc} - after canonicalisation _t3: @ BB#0: @ %bb.nph movw r12, #32768 movs r2, #0 movw r9, #16386 movt r12, #65535 LBB2_1: @ %bb @ =>This Inner Loop Header: Depth=1 eor.w r3, r0, r1 adds r2, #1 ands r3, r3, #1 it ne eorne.w r1, r1, r9 uxtb r3, r2 ubfx r1, r1, #1, #15 it ne orrne.w r1, r1, r12 ubfx r0, r0, #1, #7 cmp r3, #8 bne LBB2_1 @ BB#2: @ %bb8 uxth r0, r1 bx lr The context of this is same with the case of CoreMa, so i can't rule it out in my change. I created a review on phabricator of my proposal: https://reviews.llvm.org/D27916 Hope all the above could make the problem clear. Welcome any review and advice.Thanks! Best Regards, Jojo On 15 December 2016 at 20:10, Renato Golin <renato.golin at linaro.org> wrote:> On 8 December 2016 at 02:34, Jojo Ma <jojo.ma at linaro.org> wrote: > > It would be profitable as well if we could enable the canonicalisation on > > it. > > sequence before this canonicalisation (ARM): > > test: > > .fnstart > > @ BB#0: @ %entry > > movw r1, #65534 > > and r1, r0, r1 > > ubfx r0, r0, #1, #15 > > add r0, r0, r1, lsr #1 > > bx lr > > > > sequence after this canonicalisation: > > test: > > .fnstart > > @ BB#0: @ %entry > > ubfx r0, r0, #1, #15 > > add r0, r0, r0 > > bx lr > > > > But when I tried to expand the condition to this case, there are lots of > > various regression fails. > > Hi Jojo, > > I think this canonicalisation is correct, at least in this case, so > it's quite possible that the regressions are just bad tests, or you > actually made the code better in other places that weren't expecting > it. > > I'm adding some folks that have worked on the DAG combiner recently to > have a look, but it would be good to know what regressions were there > to see if they're really regressions (ie. worse code) or just > different/better code. > > You may also have missed a check on your code (which would wrongly > combine constants). Can you create a review on Phab of your proposed > change? > > Also, if you could list the regressions and see if they have a common > pattern (ie. "tests expecting an additional mask failed", or "the > wrong mask was applied"), it would become clearer if they are real > regressions or not. > > cheers, > --renato >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161219/3d896f51/attachment.html>
Renato Golin via llvm-dev
2016-Dec-19 16:30 UTC
[llvm-dev] visitShiftByConstant of DAGCombiner
On 19 December 2016 at 09:58, Jojo Ma <jojo.ma at linaro.org> wrote:> /home/likewise-open/SPREADTRUM/jojo.ma/jojoma/source/llvm/llvm-linaro/llvm/llvm/test/CodeGen/Thumb2/machine-licm.ll:88:10: > error: expected string not found in input > ; CHECK: movw {{(r[0-9])|(lr)}}, #32768 > ^ > <stdin>:56:2: note: scanning from here > movw r12, #32768 > ^Hi Jojo, This is just a bad check line. :) Instead of: ; CHECK: movw {{(r[0-9])|(lr)}}, #32768 it should have been: ; CHECK: movw {{(r[0-9]+)|(lr)}}, #32768 (ie. add the extra '+' at the end, so you allow more than one number). If that's the last remaining problem, I suggest you fix the CHECK line, create the tests necessary to test your pass, and make sure that the LICM code makes sense (it doesn't look worse to me, but the instructions are different, just need to make sure they have the same semantics). cheers, --renato
Hi Renato, There will be two more check fail one after another(below), after fix this one. But we could remove the check lines in my opinion, as the check fails are caused by different instructions sequence after canonicalisation. And the canonicalisation won't affect the LICM. - /home/likewise-open/SPREADTRUM/ jojo.ma/jojoma/source/llvm/llvm-linaro/llvm/llvm/test/CodeGen/Thumb2/machine-licm.ll:91:10: error: expected string not found in input ; CHECK: movw {{(r[0-9]+)|(lr)}}, #65534 ^ <stdin>:59:2: note: scanning from here movt r12, #65535 ^ - /home/likewise-open/SPREADTRUM/ jojo.ma/jojoma/source/llvm/llvm-linaro/llvm/llvm/test/CodeGen/Thumb2/machine-licm.ll:99:10: error: expected string not found in input ; CHECK: and ^ <stdin>:67:2: note: scanning from here uxtb r3, r2 ^ <stdin>:82:3: note: possible intended match here .indirect_symbol _GV ^ The diff was updated: https://reviews.llvm.org/D27916 Welcome all your comments!Thank you very much! Best Regards, Jojo On 20 December 2016 at 00:30, Renato Golin <renato.golin at linaro.org> wrote:> On 19 December 2016 at 09:58, Jojo Ma <jojo.ma at linaro.org> wrote: > > /home/likewise-open/SPREADTRUM/jojo.ma/jojoma/ > source/llvm/llvm-linaro/llvm/llvm/test/CodeGen/Thumb2/ > machine-licm.ll:88:10: > > error: expected string not found in input > > ; CHECK: movw {{(r[0-9])|(lr)}}, #32768 > > ^ > > <stdin>:56:2: note: scanning from here > > movw r12, #32768 > > ^ > > Hi Jojo, > > This is just a bad check line. :) > > Instead of: > > ; CHECK: movw {{(r[0-9])|(lr)}}, #32768 > > it should have been: > > ; CHECK: movw {{(r[0-9]+)|(lr)}}, #32768 > > (ie. add the extra '+' at the end, so you allow more than one number). > > If that's the last remaining problem, I suggest you fix the CHECK > line, create the tests necessary to test your pass, and make sure that > the LICM code makes sense (it doesn't look worse to me, but the > instructions are different, just need to make sure they have the same > semantics). > > cheers, > --renato >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161221/629291ce/attachment.html>