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

[Bob Wilson]

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  
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.

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

* 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.)

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

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.

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.

Thanks!