Typo "responisbility", otherwise looks great to me, please apply. For ARM, please just file a bugzilla suggesting that the ARM backend adopt this. Thanks Richard! -Chris On Mar 9, 2010, at 6:06 AM, Richard Osborne wrote:> On 02/03/10 00:11, Jim Grosbach wrote: >> On Mar 1, 2010, at 4:09 PM, Richard Osborne wrote: >> >>> On 01/03/10 21:14, Chris Lattner wrote: >>> >>>> On Mar 1, 2010, at 10:52 AM, Richard Osborne wrote: >>>> >>>>> On 23/02/10 14:58, Richard Osborne wrote: >>>>> >>>>> >>>>>> I've recently changed the XCore target to implement BR_JT as a jump to a >>>>>> series jumps. The jump table entries are expand inline in the function >>>>>> so there is no longer a need to emit jump tables at the end of the >>>>>> function. However the emission of jump tables at the end of a function >>>>>> is done inside the AsmPrinter base class and there seems to be no way of >>>>>> disabling this. >>>>>> >>>>>> This also seems to effect code produced for ARM Thumb2. Again each jump >>>>>> table is emitted twice - once inline in the code and once at the end of >>>>>> the function. The jump tables at the end of the function are unused. >>>>>> >>>>>> What would be the preferred way of dealing with this? Should I add a >>>>>> variable to AsmPrinter that can be set by targets to disable this >>>>>> emission at the end? >>>>>> >>>>>> >>>>> The attached patch adds a bool to MCAsmInfo that can be used to disable the emission of jump tables at the end of a function and updates the XCore target to set this. Is this OK to commit? >>>>> >>>>> >>>> Hi Richard, >>>> >>>> This shouldn't go into MCAsmInfo, it's a "lowering" issue, not a "assembly syntax" issue. ARM also has inline jump tables, how does it work? >>>> >>>> -Chris >>>> >>> I've attached the result of running llc -march=arm on test/CodeGen/XCore/switch.ll. The jump table entries are emitted twice, once inside the function after the label .LJTI1_0_0 and then again outside the function after the label .LJTI1_0. The .LJTI1_0 label at the start of the second set of jumptable entries is completely unreferenced. >>> >> Interesting. Any idea when that started happening? I'm pretty sure that didn't used to be the case. >> >> -Jim >> > On ARM the problem started after this commit: > > http://llvm.org/viewvc/llvm-project?view=rev&revision=94719 > > I've attached a new patch which adds a new jump table encoding type to indicate jump > tables are emitted inline. This fixes the problem on the XCore target. > > I can put together a patch to switch over ARM as well but I can't easily test it. After this the TargetJITInfo::hasCustomJumpTables() hook would no longer be needed and could be removed. > > -- > Richard Osborne | XMOS > http://www.xmos.com > > <jt.patch>
Thanks for reviewing this. Committed in r98255 and r98256. The bug against the ARM backend is 6581: http://llvm.org/bugs/show_bug.cgi?id=6581 On 10/03/10 21:45, Chris Lattner wrote:> Typo "responisbility", otherwise looks great to me, please apply. For ARM, please just file a bugzilla suggesting that the ARM backend adopt this. Thanks Richard! > > -Chris > > On Mar 9, 2010, at 6:06 AM, Richard Osborne wrote: > > >> On 02/03/10 00:11, Jim Grosbach wrote: >> >>> On Mar 1, 2010, at 4:09 PM, Richard Osborne wrote: >>> >>> >>>> On 01/03/10 21:14, Chris Lattner wrote: >>>> >>>> >>>>> On Mar 1, 2010, at 10:52 AM, Richard Osborne wrote: >>>>> >>>>> >>>>>> On 23/02/10 14:58, Richard Osborne wrote: >>>>>> >>>>>> >>>>>> >>>>>>> I've recently changed the XCore target to implement BR_JT as a jump to a >>>>>>> series jumps. The jump table entries are expand inline in the function >>>>>>> so there is no longer a need to emit jump tables at the end of the >>>>>>> function. However the emission of jump tables at the end of a function >>>>>>> is done inside the AsmPrinter base class and there seems to be no way of >>>>>>> disabling this. >>>>>>> >>>>>>> This also seems to effect code produced for ARM Thumb2. Again each jump >>>>>>> table is emitted twice - once inline in the code and once at the end of >>>>>>> the function. The jump tables at the end of the function are unused. >>>>>>> >>>>>>> What would be the preferred way of dealing with this? Should I add a >>>>>>> variable to AsmPrinter that can be set by targets to disable this >>>>>>> emission at the end? >>>>>>> >>>>>>> >>>>>>> >>>>>> The attached patch adds a bool to MCAsmInfo that can be used to disable the emission of jump tables at the end of a function and updates the XCore target to set this. Is this OK to commit? >>>>>> >>>>>> >>>>>> >>>>> Hi Richard, >>>>> >>>>> This shouldn't go into MCAsmInfo, it's a "lowering" issue, not a "assembly syntax" issue. ARM also has inline jump tables, how does it work? >>>>> >>>>> -Chris >>>>> >>>>> >>>> I've attached the result of running llc -march=arm on test/CodeGen/XCore/switch.ll. The jump table entries are emitted twice, once inside the function after the label .LJTI1_0_0 and then again outside the function after the label .LJTI1_0. The .LJTI1_0 label at the start of the second set of jumptable entries is completely unreferenced. >>>> >>>> >>> Interesting. Any idea when that started happening? I'm pretty sure that didn't used to be the case. >>> >>> -Jim >>> >>> >> On ARM the problem started after this commit: >> >> http://llvm.org/viewvc/llvm-project?view=rev&revision=94719 >> >> I've attached a new patch which adds a new jump table encoding type to indicate jump >> tables are emitted inline. This fixes the problem on the XCore target. >> >> I can put together a patch to switch over ARM as well but I can't easily test it. After this the TargetJITInfo::hasCustomJumpTables() hook would no longer be needed and could be removed. >> >> -- >> Richard Osborne | XMOS >> http://www.xmos.com >> >> <jt.patch> >> >-- Richard Osborne | XMOS http://www.xmos.com
Thanks, Richard. I'll have a look at that PR. -jim On Mar 11, 2010, at 7:05 AM, Richard Osborne wrote:> Thanks for reviewing this. Committed in r98255 and r98256. The bug against the ARM backend is 6581: > > http://llvm.org/bugs/show_bug.cgi?id=6581 > > On 10/03/10 21:45, Chris Lattner wrote: >> Typo "responisbility", otherwise looks great to me, please apply. For ARM, please just file a bugzilla suggesting that the ARM backend adopt this. Thanks Richard! >> >> -Chris >> >> On Mar 9, 2010, at 6:06 AM, Richard Osborne wrote: >> >> >>> On 02/03/10 00:11, Jim Grosbach wrote: >>> >>>> On Mar 1, 2010, at 4:09 PM, Richard Osborne wrote: >>>> >>>> >>>>> On 01/03/10 21:14, Chris Lattner wrote: >>>>> >>>>> >>>>>> On Mar 1, 2010, at 10:52 AM, Richard Osborne wrote: >>>>>> >>>>>> >>>>>>> On 23/02/10 14:58, Richard Osborne wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>>> I've recently changed the XCore target to implement BR_JT as a jump to a >>>>>>>> series jumps. The jump table entries are expand inline in the function >>>>>>>> so there is no longer a need to emit jump tables at the end of the >>>>>>>> function. However the emission of jump tables at the end of a function >>>>>>>> is done inside the AsmPrinter base class and there seems to be no way of >>>>>>>> disabling this. >>>>>>>> >>>>>>>> This also seems to effect code produced for ARM Thumb2. Again each jump >>>>>>>> table is emitted twice - once inline in the code and once at the end of >>>>>>>> the function. The jump tables at the end of the function are unused. >>>>>>>> >>>>>>>> What would be the preferred way of dealing with this? Should I add a >>>>>>>> variable to AsmPrinter that can be set by targets to disable this >>>>>>>> emission at the end? >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> The attached patch adds a bool to MCAsmInfo that can be used to disable the emission of jump tables at the end of a function and updates the XCore target to set this. Is this OK to commit? >>>>>>> >>>>>>> >>>>>>> >>>>>> Hi Richard, >>>>>> >>>>>> This shouldn't go into MCAsmInfo, it's a "lowering" issue, not a "assembly syntax" issue. ARM also has inline jump tables, how does it work? >>>>>> >>>>>> -Chris >>>>>> >>>>>> >>>>> I've attached the result of running llc -march=arm on test/CodeGen/XCore/switch.ll. The jump table entries are emitted twice, once inside the function after the label .LJTI1_0_0 and then again outside the function after the label .LJTI1_0. The .LJTI1_0 label at the start of the second set of jumptable entries is completely unreferenced. >>>>> >>>>> >>>> Interesting. Any idea when that started happening? I'm pretty sure that didn't used to be the case. >>>> >>>> -Jim >>>> >>>> >>> On ARM the problem started after this commit: >>> >>> http://llvm.org/viewvc/llvm-project?view=rev&revision=94719 >>> >>> I've attached a new patch which adds a new jump table encoding type to indicate jump >>> tables are emitted inline. This fixes the problem on the XCore target. >>> >>> I can put together a patch to switch over ARM as well but I can't easily test it. After this the TargetJITInfo::hasCustomJumpTables() hook would no longer be needed and could be removed. >>> >>> -- >>> Richard Osborne | XMOS >>> http://www.xmos.com >>> >>> <jt.patch> >>> >> > > > -- > Richard Osborne | XMOS > http://www.xmos.com >