<br><br><div class="gmail_quote">On Thu, Jun 25, 2009 at 6:17 PM, Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com">bob.wilson@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div style="">Hi Robert,<div><br></div><div>Evan asked me to review this patch, and I have some questions about it. I apologize for not following the discussion earlier and for hitting you with questions after you've already gone through several revisions.</div>
<div><br></div><div>LLVM provides some default behavior for handling jump tables, with the tables emitted separately from the code that uses them. The ARM backend provides its own custom handling of jump tables so that it can emit them in-line with the text. It seems to me that your patch changes the ARM backend to optionally handle jump tables in a way that is at least very similar to the default behavior, but you have implemented it without using the existing code. Did you consider implementing it using the LLVM defaults?</div>
</div></blockquote><div><br>I spend over a day trying to follow your suggestion. In the end I was not successful. Here is what Iearned:<br><br>After setting<br><br>ARMJITInfo::hasCustomJumpTables -> true<br>setOperationAction for ISD::BR_JT -> Expand<br>
<br>I needed to add a "brind" definition to ARMInstrInfo.td<br>I picked "bx" but to do a proper job one would have to take older architectures<br>into account..<br>The next thing was to to set <br> setOperationAction for ISD::JumpTable -> Custom<br>
and implement the corresponding code. <br>This code is very similar to my "outline" code as it has to materialize<br>the table start address. <br><br> I never quite got this to work, though. <br><br>Presumably what the generic code generator<br>
would to is to use this table start multiple the "switch index" by 4 load the target address and jump there. Not a very big saving in my mind.<br>And it is offset by having to touch ARMInstrInfo.td. <br>Also note, that the generic code generator must make some assumptions<br>
about the format of each jumptable entry (e.g. 32bit absolute addresses) so this cannot be changed easily afterwards.<br><br>Having the default jumtables and the out of line version diverge too much<br>and have different code path may also not be such a good idea.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style=""><div></div><div><br></div><div>Using TargetAsmInfo::JumpTableDataSection is one piece of that, but there is more. As a start, you could change ARMJITInfo::hasCustomJumpTables and the "Custom" argument to setOperationAction for ISD::BR_JT in the ARMTargetLowering constructor to be conditional on your new flag. I'm sure there is more required than that, but maybe that will get you started.</div>
<div><br></div><div>Otherwise, there are some problems with your custom handling for out-of-line jump tables. The most serious of these is in ARMAsmPrinter::printJTBlockOperand: for out-of-line jump tables, you emit a ".text" directive to switch back to the text section. If the current section is not ".text", however, this will break. I believe you ought to use SwitchToTextSection instead. Earlier in that same method, I think you should also use SwitchToDataSection and EmitAlignment. But, again, rather than tweaking this code, I would prefer to find a way to use the existing LLVM code for this.</div>
<div></div></div></blockquote><div><br>I changed all of these as per your request.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div style=""><div><br></div><div>Why did you change the default value for JumpTableDataSection? Jump tables really should not go into .data. That is a security hole. They should be read-only.</div></div></blockquote><div>
<br>I changed that too so the data goes into the default .rodata now.<br>I am not sure where .rodata lives. if it lives in the text *segment* <br>you get better protection but the code wont be PIC if .rodata<br>goes into the data segment it is the other way round.<br>
<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style=""><div></div><div><br></div><div>When you create global symbols for the jump tables, you specify a ".T" name:</div>
<div><br></div><div><div><span style="white-space: pre;"> </span>new ARMConstantPoolValue(".T", Num, ARMCP::CPDataSegmentJumpTable);</div><div><br></div><div>Why ".T"?</div><div></div></div></div></blockquote>
<div><br>T as in Table, I do not care much about the naming. If you have a better proposal I will go with that.<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div style=""><div><div><br></div></div><div>And, by the way, I noticed that you removed a "blank" comment line in ARMISelLowering.h. I think I have done that myself, but I recently discovered that the extra lines are intentional. Doxygen recognizes C++ comments beginning with 3 slashes but you have to have at least 2 lines of them or doxygen will not recognize them.</div>
<div></div></div></blockquote><div><br>* fixed and added artificial line break to keep this from happening again.<br> <br><br>Happy holidays!<br>Robert<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div style=""><div><br></div><div><div><div><div></div><div class="h5"><div>On Jun 24, 2009, at 2:20 PM, robert muth wrote:</div><br></div></div><blockquote type="cite"><div><div></div><div class="h5">Evan:<br><br>Sorry for the late follow up, I was out of town last week.<br>
Enclosed please find the updated patch including all<br>your suggestions and a dejagnus test.<br><br>Robert<br><br><div class="gmail_quote">On Thu, Jun 11, 2009 at 2:27 PM, Evan Cheng <span dir="ltr"><<a href="mailto:evan.cheng@apple.com" target="_blank">evan.cheng@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div></div><div><br> On Jun 8, 2009, at 2:42 PM, robert muth wrote:<br> <br> > On Sun, Jun 7, 2009 at 11:53 PM, Evan Cheng <<a href="mailto:evan.cheng@apple.com" target="_blank">evan.cheng@apple.com</a>><br>
> wrote:<br> >><br> >> On Jun 7, 2009, at 6:59 AM, robert muth wrote:<br> >><br> >>> On Sat, Jun 6, 2009 at 4:51 PM, Evan Cheng<<a href="mailto:evan.cheng@apple.com" target="_blank">evan.cheng@apple.com</a>><br>
>>> wrote:<br> >>>> +cl::opt<std::string> FlagJumpTableSection("jumptable-section",<br> >>>> +<br> >>>> cl::init(".data.jtab"));<br> >>>> +<br>
>>><br> >>> I thought it would be nice to group all the jumptables together.<br> >>> But as long as it stays configurable, I am fine to change the<br> >>> default<br> >>> to ".data".<br>
>><br> >> There is already a TargetAsmInfo::JumpTableDataSection. Why not just<br> >> use that?<br> ><br> > Nice find. I will use that and possible change the current setting,<br> > ".const", if it does not work,<br>
><br> >>><br> >>>> Is this necessary? Why not just put it in the normal data section?<br> >>>> Also "outline" jumptable seems like a strange term. Can you think<br> >>>> of<br>
>>>> something else?<br> >>><br> >>><br> >>> Yes, that is a tough one. How about "NonInline" instead.<br> >><br> >> Or "OutOfLine"?<br> ><br> > That works for me.<br>
><br> >> Please add a test case as well. Thanks,<br> ><br> > I am not sure how to go about testing.<br> <br> </div></div>For this please just add a test case to the llvm dejagnu tests.<br> <br> thanks,<br>
<font color="#888888"><br> Evan<br> </font><div><div></div><div><br> ><br> > I have a script that compiles a bunch of test<br> > programs (gnu c torture test, etc.) and then runs the executables<br> > on qemu. I run this script with and without my flags and<br>
> make sure that I do not introduce any new problems<br> > -- there are currently plenty of vararg issues.<br> ><br> > I was thinking of sending this script to the list<br> > or maybe checking it into the llvm tree.<br>
><br> > The key is that whatever tests there are they should just<br> > be run with and without the new flag.<br> > How do you run backend tests?<br> ><br> > Robert<br> ><br> ><br> ><br> ><br>
>> Evan<br> >><br> >>><br> >>> Robert<br> >>><br> >>>> Thanks,<br> >>>> Evan<br> >>>><br> >>>> Sent from my iPhone<br> >>>> On Jun 2, 2009, at 6:26 PM, robert muth <<a href="mailto:robert@muth.org" target="_blank">robert@muth.org</a>> wrote:<br>
>>>><br> >>>> Hi:<br> >>>><br> >>>> This is my first patch submission. Hopefully, this is the proper<br> >>>> the<br> >>>> protocol.<br> >>>> Attached is a patch for the llc ARM backend:<br>
>>>><br> >>>> Added mechanism to generate switch table in a data section<br> >>>> rather than having it interleaved with the code.<br> >>>> This is controlled by command line flags and off by default.<br>
>>>> Also, tried to document and improve the code where I modified it.<br> >>>><br> >>>> Robert<br> >>>><br> >>>> <llc.patch.txt><br> >>>><br>
>>>> _______________________________________________<br> >>>> LLVM Developers mailing list<br> >>>> <a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br> >>>><br> >>>> _______________________________________________<br>
>>>> LLVM Developers mailing list<br> >>>> <a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br> >>>><br> >>>><br> >>><br> >>> _______________________________________________<br>
>>> LLVM Developers mailing list<br> >>> <a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br> >><br> >> _______________________________________________<br>
>> LLVM Developers mailing list<br> >> <a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
>><br> ><br> > _______________________________________________<br> > LLVM Developers mailing list<br> > <a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br> <br> _______________________________________________<br> LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
</div></div></blockquote></div><br> </div></div><span><llc.patch.2.txt></span>_______________________________________________<div class="im"><br>LLVM Developers mailing list<br><a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br></div></blockquote></div><br></div></div><br>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br></blockquote></div><br>