Bob:<br><br>a new patch is attached. 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>more info inline<br><br><div class="gmail_quote">On Mon, Jul 27, 2009 at 2:24 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 class="im"><br>
On Jul 23, 2009, at 12:10 PM, robert muth wrote:<br>
> Bob:<br>
><br>
> Thanks for cleaning this up. I like the new patch much better than<br>
> the old one.<br>
> Teaching the (abstract) ConstantValue class about jumptable indices<br>
> is a little<br>
> bit ugly but I do not see any better solution without massive<br>
> refactoring.<br>
> I have added TODOs here and elsewhere and plan to address them in<br>
> future<br>
> patches. I also added a test and pcleaned up a few things.<br>
<br>
<br>
</div>Maybe I'm missing something but it looks like this is basically the<br>
same as the sample code that I sent back to you. Aside from the TODO<br>
comments, it looks like all you changed was to move the new code in<br>
BranchFolding into a new UpdateLiveSetUsingConstantPoolAndMapIndices<br>
function, and I don't particularly like that change -- the new code is<br>
not that big and it is essentially the same code as in the preceding<br>
loop. If anything, maybe it would be possible to refactor those two</blockquote><div><br>* undid my refactoring.<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>
loops to share some code? I don't see the point in just moving one of<br>
them out to a separate function with a big name.<br>
<br>
As I wrote before, there are a few things that need to be addressed<br>
before we can take the patch. I'll repeat them here for clarity:<br>
<br>
* It needs to work on Thumb and Thumb2. I'm not even sure if the<br>
"brind" pattern I added is the right thing for ARM mode.<br>
</blockquote><div><br>added a brind instruction for thumb, but it seems that jump tables are broken<br>on thumb even with "inlined tables"<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>
* The JIT code emitter is definitely broken with this. Unless there<br>
is a fundamental reason why this is hard, it should be fixed.<br>
</blockquote><div><br>as per our private conversation did not address this <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>
* The testcase should use the new FileCheck format. (I think I<br>
phrased this as a suggestion before, but upon looking more closely at<br>
the test, it seems like the sort of test that is not well-suited to<br>
using grep.)<br>
</blockquote><div><br>I made small change to look for "section" rather than "rodata" to make the test more portable.<br>As per our private conversation did not switch to "FileCheck format" because it does<br>
not seem to support multiple passes over the same file.<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>
You didn't respond to my comment about the command-line option name.<br>
What do you think of "no-inline-jumptables"?<br>
</blockquote><div><br>* command line naming is following your proposal<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>
How do you plan to test this? The simple testcase is a good start.<br>
Beyond that, I'd like to see you (temporarily) enable this by default<br>
and run through the llvm testsuite.<br>
</blockquote><div><br>* did a *lot* testing using gnu torture test, overall I ran 3x1000 small c programs on qemu<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>
I also see you added a comment: "TODO: this does not currently work<br>
for PIC mode: the code works correctly but the table still ends up in<br>
the .text section." I don't remember seeing that behavior, but maybe<br>
I missed it. Please investigate why this is happening.<br>
</blockquote><div><br>* reworded this but it seems to agree with your initial findings. <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>
Thanks!<br>
<div><div></div><div class="h5">_______________________________________________<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>
</div></div></blockquote></div><br>