<br><br><div class="gmail_quote">On Tue, Jul 14, 2009 at 6:48 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=""><br><div><div class="im"><div>On Jul 2, 2009, at 10:48 AM, robert muth wrote:</div></div><div class="im"><blockquote type="cite"><div class="gmail_quote"><div>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.</div></div></blockquote>
<div><br></div></div>Sorry for the slow response. Since it wasn't clear what problems you ran into, and since I wasn't very familiar with LLVM's handling of jump tables, I wanted to have a look at it myself to see what the issues were.</div>
<div><br></div><div>I was able to make it work after fixing one problem: the branch folder removes "dead" jump tables and it doesn't scan the constant pool for jump table references. I had to add some code to the branch folder and the MachineConstantPoolValue class to fix that. I haven't yet done much testing of this but it looks like it's basically working.</div>
<div><br></div><div>Since I had to write the code to try this out, I'm sending my revised patch back to you. There are still a few issues to sort out before committing this:</div><div><br></div><div>* I followed your lead and added a "brind" pattern using "bx". I think this works on older architectures, too. What issue were you thinking of with regard to supporting older architectures? I didn't even look at Thumb or Thumb2, but we'll need something for them.</div>
<div><br></div><div>* I saw some earlier discussion about the command-line option name. You had "outline-jumptables" and Evan had suggested "OutOfLine". I don't particularly like either one (sorry, Evan). How about "no-inline-jumptables"? I prefer that because it captures the sense that this option is disabling an ARM-specific feature (the inline jumptables) and changing back to the default.</div>
<div><br></div><div>* I didn't look at the JIT code emitter at all. That will need to be fixed.</div><div><br></div><div>* It would be great to change the testcase to use the new FileCheck format.</div><div><br></div>
<div>I've attached my revised patch to show you how I made this work. I've also attached a copy of your last patch with embedded comments (search for "bob>"). Let me know what you think and if you can take a look at addressing some of the open issues above.</div>
<div><br></div><div><div class="im"><blockquote type="cite"><div class="gmail_quote"><div>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>
</div></div></blockquote><div><br></div></div>I'm not so much concerned about savings in the quantity of code as I am in keeping the ARM backend from diverging too far from the other LLVM backends. Sometimes we have to do that but it is an ongoing maintenance burden, and in this case, it seems like we can easily avoid it.</div>
<div><br></div><div>It also helps leverage the existing code. For example, I didn't look too closely, but with my patch, compiling with -relocation-model=pic seemed to do the right thing.</div><div><div class="im"><div>
<br></div><blockquote type="cite"><div class="gmail_quote"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div><font color="#000000"><br>
</font></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>
</div></div></blockquote><div><br></div></div>If you compile with -relocation-model=pic, then the jump table contains offsets relative to the start of the table. The tables live in .rodata (or even in .text itself, but not inline in the code) which is in the same segment as .text and thus the code is still PIC.</div>
</div></blockquote><div><br><br><br>Bob:<br><br>Thanks for cleaning this up. I like the new patch much better than the old one.<br>Teaching the (abstract) ConstantValue class about jumptable indices is a little<br>bit ugly but I do not see any better solution without massive refactoring.<br>
I have added TODOs here and elsewhere and plan to address them in future<br>patches. I also added a test and pcleaned up a few things.<br><br>The new patch is attached.<br>I also uploaded it to <br><br> <a href="http://codereview.appspot.com/96065/show" target="_blank">http://codereview.appspot.com/96065/show</a><br>
<br>which is more convenient for browsing.<br>(you will have to register with an arbitrary pair<br>of email address and password --if you have a gmail<br>account you can use that)<br>Cheers,<br><br>Robert<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;">
<br><div style=""></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>