[LLVMdev] patch for llc/ARM: added mechanism to move switch tables from .text -> .data; also cleanup and documentation

[robert muth]

Bob:

a new patch is attached. I also uploaded it to

 http://codereview.appspot.com/96065/show

more info inline

On Mon, Jul 27, 2009 at 2:24 PM, Bob Wilson <bob.wilson at apple.com> wrote:

>
> On Jul 23, 2009, at 12:10 PM, robert muth wrote:
> > Bob:
> >
> > Thanks for cleaning this up. I like the new patch much better than
> > the old one.
> > Teaching the (abstract) ConstantValue class about jumptable indices
> > is a little
> > bit ugly but I do not see any better solution without massive
> > refactoring.
> > I have added TODOs  here and elsewhere and plan to address them in
> > future
> > patches. I also added a test and pcleaned up a few things.
>
>
> Maybe I'm missing something but it looks like this is basically the
> same as the sample code that I sent back to you.  Aside from the TODO
> comments, it looks like all you changed was to move the new code in
> BranchFolding into a new UpdateLiveSetUsingConstantPoolAndMapIndices
> function, and I don't particularly like that change -- the new code is
> not that big and it is essentially the same code as in the preceding
> loop.  If anything, maybe it would be possible to refactor those two


* undid my refactoring.


>
> loops to share some code?  I don't see the point in just moving one of
> them out to a separate function with a big name.
>
> As I wrote before, there are a few things that need to be addressed
> before we can take the patch.  I'll repeat them here for clarity:
>
> * It needs to work on Thumb and Thumb2.  I'm not even sure if the
> "brind" pattern I added is the right thing for ARM mode.
>

added a brind instruction for thumb, but it seems that jump tables are
broken
on thumb even with "inlined tables"


>
> * The JIT code emitter is definitely broken with this.  Unless there
> is a fundamental reason why this is hard, it should be fixed.
>

as per our private conversation did not address this

>
> * The testcase should use the new FileCheck format.  (I think I
> phrased this as a suggestion before, but upon looking more closely at
> the test, it seems like the sort of test that is not well-suited to
> using grep.)
>

I made small change to look for "section" rather than "rodata" to make the
test more portable.
As per our private conversation did not switch to "FileCheck format" because
it does
not seem to support multiple passes over the same file.


>
> You didn't respond to my comment about the command-line option name.
> What do you think of "no-inline-jumptables"?
>

* command line naming is following  your proposal

>
> How do you plan to test this?  The simple testcase is a good start.
> Beyond that, I'd like to see you (temporarily) enable this by default
> and run through the llvm testsuite.
>

* did a *lot* testing using gnu torture test, overall I ran 3x1000 small c
programs on qemu


> I also see you added a comment: "TODO: this does not currently work
> for PIC mode: the code works correctly but the table still ends up in
> the .text section."  I don't remember seeing that behavior, but maybe
> I missed it.  Please investigate why this is happening.
>

* reworded this but it seems to agree with your initial findings.


> Thanks!
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090806/db005de1/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-jt.patch
Type: text/x-diff
Size: 17913 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090806/db005de1/attachment.patch>