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> Given that MCAsmInfo is the wrong place would it make sense to describe it instead as a separate type of jump table encoding and add it to the MachineJumpTableInfo::JTEntryKind enum?
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 -------------- next part -------------- A non-text attachment was scrubbed... Name: jt.patch Type: text/x-patch Size: 4312 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100309/09328c5e/attachment.bin>
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>