[LLVMdev] patch for llc/ARM: added mechanism to move switch tables from .text -> .data; also cleanup and documentation
Bob Wilson
bob.wilson at apple.com
Thu Jun 25 15:17:19 PDT 2009
Hi Robert,
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.
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?
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.
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.
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.
When you create global symbols for the jump tables, you specify a ".T"
name:
new ARMConstantPoolValue(".T", Num, ARMCP::CPDataSegmentJumpTable);
Why ".T"?
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.
On Jun 24, 2009, at 2:20 PM, robert muth wrote:
> Evan:
>
> Sorry for the late follow up, I was out of town last week.
> Enclosed please find the updated patch including all
> your suggestions and a dejagnus test.
>
> Robert
>
> On Thu, Jun 11, 2009 at 2:27 PM, Evan Cheng <evan.cheng at apple.com>
> wrote:
>
> On Jun 8, 2009, at 2:42 PM, robert muth wrote:
>
> > On Sun, Jun 7, 2009 at 11:53 PM, Evan Cheng <evan.cheng at apple.com>
> > wrote:
> >>
> >> On Jun 7, 2009, at 6:59 AM, robert muth wrote:
> >>
> >>> On Sat, Jun 6, 2009 at 4:51 PM, Evan Cheng<evan.cheng at apple.com>
> >>> wrote:
> >>>> +cl::opt<std::string> FlagJumpTableSection("jumptable-section",
> >>>> +
> >>>> cl::init(".data.jtab"));
> >>>> +
> >>>
> >>> I thought it would be nice to group all the jumptables together.
> >>> But as long as it stays configurable, I am fine to change the
> >>> default
> >>> to ".data".
> >>
> >> There is already a TargetAsmInfo::JumpTableDataSection. Why not
> just
> >> use that?
> >
> > Nice find. I will use that and possible change the current setting,
> > ".const", if it does not work,
> >
> >>>
> >>>> Is this necessary? Why not just put it in the normal data
> section?
> >>>> Also "outline" jumptable seems like a strange term. Can you think
> >>>> of
> >>>> something else?
> >>>
> >>>
> >>> Yes, that is a tough one. How about "NonInline" instead.
> >>
> >> Or "OutOfLine"?
> >
> > That works for me.
> >
> >> Please add a test case as well. Thanks,
> >
> > I am not sure how to go about testing.
>
> For this please just add a test case to the llvm dejagnu tests.
>
> thanks,
>
> Evan
>
> >
> > I have a script that compiles a bunch of test
> > programs (gnu c torture test, etc.) and then runs the executables
> > on qemu. I run this script with and without my flags and
> > make sure that I do not introduce any new problems
> > -- there are currently plenty of vararg issues.
> >
> > I was thinking of sending this script to the list
> > or maybe checking it into the llvm tree.
> >
> > The key is that whatever tests there are they should just
> > be run with and without the new flag.
> > How do you run backend tests?
> >
> > Robert
> >
> >
> >
> >
> >> Evan
> >>
> >>>
> >>> Robert
> >>>
> >>>> Thanks,
> >>>> Evan
> >>>>
> >>>> Sent from my iPhone
> >>>> On Jun 2, 2009, at 6:26 PM, robert muth <robert at muth.org> wrote:
> >>>>
> >>>> Hi:
> >>>>
> >>>> This is my first patch submission. Hopefully, this is the proper
> >>>> the
> >>>> protocol.
> >>>> Attached is a patch for the llc ARM backend:
> >>>>
> >>>> Added mechanism to generate switch table in a data section
> >>>> rather than having it interleaved with the code.
> >>>> This is controlled by command line flags and off by default.
> >>>> Also, tried to document and improve the code where I modified it.
> >>>>
> >>>> Robert
> >>>>
> >>>> <llc.patch.txt>
> >>>>
> >>>> _______________________________________________
> >>>> LLVM Developers mailing list
> >>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >>>>
> >>>> _______________________________________________
> >>>> LLVM Developers mailing list
> >>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >>>>
> >>>>
> >>>
> >>> _______________________________________________
> >>> LLVM Developers mailing list
> >>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >>
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >>
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
> <llc.patch.2.txt>_______________________________________________
> 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/20090625/c6d93956/attachment.html>
More information about the llvm-dev
mailing list