robert muth
2009-Jun-24 21:20 UTC
[LLVMdev] patch for llc/ARM: added mechanism to move switch tables from .text -> .data; also cleanup and documentation
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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090624/01802d9c/attachment.html> -------------- next part -------------- Index: test/CodeGen/ARM/switch.ll ==================================================================--- test/CodeGen/ARM/switch.ll (revision 0) +++ test/CodeGen/ARM/switch.ll (revision 0) @@ -0,0 +1,82 @@ +; Note: jump table has 7 entries +; RUN: llvm-as < %s | llc -march=arm | \ +; RUN: grep .long | count 7 +; Note: no section change +; RUN: llvm-as < %s | llc -march=arm | \ +; RUN: grep .text | count 1 +; Note: no data section +; RUN: llvm-as < %s | llc -march=arm | \ +; RUN: not grep .data +; +; Note: jump table has 7 entries + 1 entry for the table start +; RUN: llvm-as < %s | llc -march=arm -outline-jumptables | \ +; RUN: grep .long | count 8 +; Note one section change +; RUN: llvm-as < %s | llc -march=arm -outline-jumptables | \ +; RUN: grep .text | count 2 +; Note: has data section +; RUN: llvm-as < %s | llc -march=arm -outline-jumptables | \ +; RUN: grep .data | count 1 + + +define i32 @f(i32 %i) nounwind { +entry: + %i_addr = alloca i32 ; <i32*> [#uses=2] + %retval = alloca i32 ; <i32*> [#uses=2] + %0 = alloca i32 ; <i32*> [#uses=8] + %"alloca point" = bitcast i32 0 to i32 ; <i32> [#uses=0] + store i32 %i, i32* %i_addr + %1 = load i32* %i_addr, align 4 ; <i32> [#uses=1] + switch i32 %1, label %bb7 [ + i32 -2, label %bb + i32 -1, label %bb1 + i32 0, label %bb2 + i32 1, label %bb3 + i32 2, label %bb4 + i32 3, label %bb5 + i32 4, label %bb6 + ] + +bb: ; preds = %entry + store i32 33, i32* %0, align 4 + br label %bb8 + +bb1: ; preds = %entry + store i32 0, i32* %0, align 4 + br label %bb8 + +bb2: ; preds = %entry + store i32 7, i32* %0, align 4 + br label %bb8 + +bb3: ; preds = %entry + store i32 4, i32* %0, align 4 + br label %bb8 + +bb4: ; preds = %entry + store i32 3, i32* %0, align 4 + br label %bb8 + +bb5: ; preds = %entry + store i32 15, i32* %0, align 4 + br label %bb8 + +bb6: ; preds = %entry + store i32 9, i32* %0, align 4 + br label %bb8 + +bb7: ; preds = %entry + store i32 999, i32* %0, align 4 + br label %bb8 + +bb8: ; preds = %bb6, %bb5, %bb4, %bb3, %bb2, %bb1, %bb + %2 = load i32* %0, align 4 ; <i32> [#uses=1] + store i32 %2, i32* %retval, align 4 + br label %return + +return: ; preds = %bb8 + %retval9 = load i32* %retval ; <i32> [#uses=1] + ret i32 %retval9 +} + + Index: lib/Target/ARM/ARMISelLowering.h ==================================================================--- lib/Target/ARM/ARMISelLowering.h (revision 74072) +++ lib/Target/ARM/ARMISelLowering.h (working copy) @@ -202,9 +202,10 @@ /// make the right decision when generating code for different targets. const ARMSubtarget *Subtarget; - /// ARMPCLabelIndex - Keep track the number of ARM PC labels created. - /// + /// ARMPCLabelIndex - Keep track of the number of ARM PC labels created. unsigned ARMPCLabelIndex; + /// ARMJumpTableIndex - Keep track of the number ofJump Tables + unsigned ARMJumpTableIndex; void addTypeForNEON(MVT VT, MVT PromotedLdStVT, MVT PromotedBitwiseVT); void addDRTypeForNEON(MVT VT); Index: lib/Target/ARM/ARMConstantPoolValue.h ==================================================================--- lib/Target/ARM/ARMConstantPoolValue.h (revision 74072) +++ lib/Target/ARM/ARMConstantPoolValue.h (working copy) @@ -25,6 +25,7 @@ enum ARMCPKind { CPValue, CPNonLazyPtr, + CPDataSegmentJumpTable, CPStub }; } @@ -54,7 +55,6 @@ ARMConstantPoolValue(GlobalValue *GV, ARMCP::ARMCPKind Kind, const char *Modifier); - GlobalValue *getGV() const { return GV; } const char *getSymbol() const { return S; } const char *getModifier() const { return Modifier; } @@ -63,6 +63,9 @@ unsigned getLabelId() const { return LabelId; } bool isNonLazyPointer() const { return Kind == ARMCP::CPNonLazyPtr; } bool isStub() const { return Kind == ARMCP::CPStub; } + bool isValue() const { return Kind == ARMCP::CPValue; } + bool isDataSegmentJumpTable() const { + return Kind == ARMCP::CPDataSegmentJumpTable; } unsigned char getPCAdjustment() const { return PCAdjust; } virtual int getExistingMachineCPValue(MachineConstantPool *CP, Index: lib/Target/ARM/AsmPrinter/ARMAsmPrinter.cpp ==================================================================--- lib/Target/ARM/AsmPrinter/ARMAsmPrinter.cpp (revision 74072) +++ lib/Target/ARM/AsmPrinter/ARMAsmPrinter.cpp (working copy) @@ -33,13 +33,17 @@ #include "llvm/ADT/Statistic.h" #include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringSet.h" +#include "llvm/Support/CommandLine.h" #include "llvm/Support/Compiler.h" #include "llvm/Support/Mangler.h" #include "llvm/Support/MathExtras.h" #include "llvm/Support/raw_ostream.h" #include <cctype> +#include <sstream> using namespace llvm; +extern cl::opt<bool> FlagOutlineJumpTables; + STATISTIC(EmittedInsts, "Number of machine instrs printed"); namespace { @@ -141,6 +145,7 @@ /// EmitMachineConstantPoolValue - Print a machine constantpool value to /// the .s file. virtual void EmitMachineConstantPoolValue(MachineConstantPoolValue *MCPV) { + // NOTE: A lot of this code is replicated in ARMConstantPoolValue::print printDataDirective(MCPV->getType()); ARMConstantPoolValue *ACPV = static_cast<ARMConstantPoolValue*>(MCPV); @@ -157,8 +162,13 @@ } else if (ACPV->isStub()) { FnStubs.insert(Name); printSuffixedName(Name, "$stub"); - } else + } else if (ACPV->isDataSegmentJumpTable()) { + // requires synchronization with code (grep for "$jumptable$") + O << Name << "$jumptable$" << ACPV->getLabelId(); + } else { + assert(ACPV->isValue() && "unknown CP kind"); O << Name; + } if (ACPV->hasModifier()) O << "(" << ACPV->getModifier() << ")"; if (ACPV->getPCAdjustment() != 0) { O << "-(" << TAI->getPrivateGlobalPrefix() << "PC" @@ -757,12 +767,31 @@ } } + void ARMAsmPrinter::printJTBlockOperand(const MachineInstr *MI, int OpNo) { - const MachineOperand &MO1 = MI->getOperand(OpNo); - const MachineOperand &MO2 = MI->getOperand(OpNo+1); // Unique Id - unsigned JTI = MO1.getIndex(); - O << TAI->getPrivateGlobalPrefix() << "JTI" << getFunctionNumber() - << '_' << JTI << '_' << MO2.getImm() << ":\n"; + std::stringstream prefix; + + const unsigned JTI = MI->getOperand(OpNo).getIndex(); + const unsigned uid = MI->getOperand(OpNo+1).getImm(); + + if (FlagOutlineJumpTables) { + // needs to be synchronized with ARMConstantPoolValue.cpp + prefix << ".T$jumptable$" << uid; + } else { + // needs to be synchronized with other places in this file + prefix << TAI->getPrivateGlobalPrefix() << "JTI" << getFunctionNumber() + << '_' << JTI << '_' << uid; + } + + if (FlagOutlineJumpTables) { + // switch out of text section + O << TAI->getJumpTableDataSection(); + O << ".align 4\n"; + O << "\n\n"; + } + + // the table label + O << prefix.str() << ":\n"; const char *JTEntryDirective = TAI->getJumpTableDirective(); if (!JTEntryDirective) @@ -777,24 +806,28 @@ for (unsigned i = 0, e = JTBBs.size(); i != e; ++i) { MachineBasicBlock *MBB = JTBBs[i]; if (UseSet && JTSets.insert(MBB).second) - printPICJumpTableSetLabel(JTI, MO2.getImm(), MBB); + printPICJumpTableSetLabel(JTI, uid, MBB); O << JTEntryDirective << ' '; if (UseSet) - O << TAI->getPrivateGlobalPrefix() << getFunctionNumber() - << '_' << JTI << '_' << MO2.getImm() - << "_set_" << MBB->getNumber(); + O << prefix.str() << "_set_" << MBB->getNumber(); else if (TM.getRelocationModel() == Reloc::PIC_) { printBasicBlockLabel(MBB, false, false, false); // If the arch uses custom Jump Table directives, don't calc relative to JT if (!TAI->getJumpTableDirective()) - O << '-' << TAI->getPrivateGlobalPrefix() << "JTI" - << getFunctionNumber() << '_' << JTI << '_' << MO2.getImm(); + O << '-' << prefix.str(); } else printBasicBlockLabel(MBB, false, false, false); if (i != e-1) O << '\n'; } + + if (FlagOutlineJumpTables) { + // switch back into the text section + O << "\n\n"; + O << ".text\n"; + O << "\n\n"; + } } Index: lib/Target/ARM/ARMTargetAsmInfo.cpp ==================================================================--- lib/Target/ARM/ARMTargetAsmInfo.cpp (revision 74072) +++ lib/Target/ARM/ARMTargetAsmInfo.cpp (working copy) @@ -52,6 +52,7 @@ SetDirective = "\t.set\t"; ProtectedDirective = NULL; HasDotTypeDotSizeDirective = false; + JumpTableDataSection = "\t.section .data,\"aMS\",%progbits,1\n"; SupportsDebugInformation = true; } @@ -86,6 +87,7 @@ StaticCtorsSection = "\t.section .ctors,\"aw\",%progbits"; StaticDtorsSection = "\t.section .dtors,\"aw\",%progbits"; } + JumpTableDataSection = "\t.section .data,\"aMS\",%progbits,1\n"; SupportsDebugInformation = true; } Index: lib/Target/ARM/ARMISelLowering.cpp ==================================================================--- lib/Target/ARM/ARMISelLowering.cpp (revision 74072) +++ lib/Target/ARM/ARMISelLowering.cpp (working copy) @@ -36,9 +36,16 @@ #include "llvm/CodeGen/SelectionDAG.h" #include "llvm/Target/TargetOptions.h" #include "llvm/ADT/VectorExtras.h" +#include "llvm/Support/CommandLine.h" #include "llvm/Support/MathExtras.h" + using namespace llvm; + + +cl::opt<bool> FlagOutlineJumpTables("outline-jumptables", + cl::desc("move jumptables from text to data")); + static bool CC_ARM_APCS_Custom_f64(unsigned &ValNo, MVT &ValVT, MVT &LocVT, CCValAssign::LocInfo &LocInfo, ISD::ArgFlagsTy &ArgFlags, @@ -1686,27 +1693,119 @@ return Res; } -SDValue ARMTargetLowering::LowerBR_JT(SDValue Op, SelectionDAG &DAG) { + + +// Similar to LowerBR_JT_Inline except that the jumptable +// is moved to the data segment. +// This causes a extra load to access the table but keeps the +// text segment small and avoids some issues with generating PIC +// Needs to be activated with a commandline flag. +static SDValue LowerBR_JT_OutOfLine(SDValue Op, + SelectionDAG &DAG, + unsigned Num, + MVT PTy) { + SDValue Chain = Op.getOperand(0); - SDValue Table = Op.getOperand(1); - SDValue Index = Op.getOperand(2); - DebugLoc dl = Op.getDebugLoc(); - MVT PTy = getPointerTy(); - JumpTableSDNode *JT = cast<JumpTableSDNode>(Table); + const JumpTableSDNode *JT = cast<JumpTableSDNode>(Op.getOperand(1)); + const SDValue Index = Op.getOperand(2); + + + const DebugLoc dl = Op.getDebugLoc(); + + const SDValue UId = DAG.getConstant(Num, PTy); + const SDValue JTI = DAG.getTargetJumpTable(JT->getIndex(), PTy); + + // create a new global symbol for the jumptable + ARMConstantPoolValue *CPV = new ARMConstantPoolValue(".T", Num, + ARMCP::CPDataSegmentJumpTable); + const SDValue CPAddr = DAG.getTargetConstantPool(CPV, PTy, 4); + + // An ARM idiosyncrasy: wrap each constant pool entry before accessing it + const SDValue Wrapper = DAG.getNode(ARMISD::Wrapper, dl, MVT::i32, CPAddr); + + // Load Table start from constan pool + const SDValue Table = DAG.getLoad(PTy, dl, DAG.getEntryNode(), Wrapper, NULL, 0); + + // table entries are 4 bytes, so multiple index by 4 + const SDValue ScaledIndex = DAG.getNode(ISD::MUL, dl, PTy, Index, DAG.getConstant(4, PTy)); + + // add scaled index to table beginning + const SDValue TabEntryAddr = DAG.getNode(ISD::ADD, dl, PTy, ScaledIndex, Table); + const SDValue JumpTarget = DAG.getLoad(PTy, dl, Chain, TabEntryAddr, NULL, 0); + + return DAG.getNode(ARMISD::BR_JT, dl, MVT::Other, Chain, JumpTarget, JTI, UId); +} + + +static SDValue LowerBR_JT_Inline(SDValue Op, + SelectionDAG &DAG, + bool isPIC, + MVT PTy) { + // The Jumptable idiom we are aiming for looks somthing like this: + // + // .set PCRELV0, (.LJTI9_0_0-(.LPCRELL0+8)) + // .LPCRELL0: + // add r3, pc, #PCRELV0 + // ldr pc, [r3, +r0, lsl #2] + // .LJTI9_0_0: + // .long .LBB9_2 + // .long .LBB9_5 + // .long .LBB9_7 + // .long .LBB9_4 + // .long .LBB9_8 + // + // In pic mode the table entries are relative to table beginning + // requiring and extra addition + // + // The code generation logic for ARMISD::BR_JT will also + // emit the table (c.f. ARMAsmPrinter::printJTBlockOperand()) + // Also check "def BR_JTm" in ARMInstrInfo.td + + // allocate constant pool entry ARMFunctionInfo *AFI = DAG.getMachineFunction().getInfo<ARMFunctionInfo>(); - SDValue UId = DAG.getConstant(AFI->createJumpTableUId(), PTy); - SDValue JTI = DAG.getTargetJumpTable(JT->getIndex(), PTy); - Table = DAG.getNode(ARMISD::WrapperJT, dl, MVT::i32, JTI, UId); - Index = DAG.getNode(ISD::MUL, dl, PTy, Index, DAG.getConstant(4, PTy)); - SDValue Addr = DAG.getNode(ISD::ADD, dl, PTy, Index, Table); - bool isPIC = getTargetMachine().getRelocationModel() == Reloc::PIC_; - Addr = DAG.getLoad(isPIC ? (MVT)MVT::i32 : PTy, dl, - Chain, Addr, NULL, 0); - Chain = Addr.getValue(1); - if (isPIC) - Addr = DAG.getNode(ISD::ADD, dl, PTy, Addr, Table); - return DAG.getNode(ARMISD::BR_JT, dl, MVT::Other, Chain, Addr, JTI, UId); + + SDValue Chain = Op.getOperand(0); + + const JumpTableSDNode *JT = cast<JumpTableSDNode>(Op.getOperand(1)); + const SDValue Index = Op.getOperand(2); + const DebugLoc dl = Op.getDebugLoc(); + + + const SDValue UId = DAG.getConstant(AFI->createJumpTableUId(), PTy); + + const SDValue JTI = DAG.getTargetJumpTable(JT->getIndex(), PTy); + + // this uses pcrel magic to materialize the table start address + const SDValue Table = DAG.getNode(ARMISD::WrapperJT, dl, MVT::i32, JTI, UId); + + // table entries are 4 bytes, so multiple index by 4 + const SDValue ScaledIndex = DAG.getNode(ISD::MUL, dl, PTy, Index, DAG.getConstant(4, PTy)); + + // add scaled index to table beginning + const SDValue TabEntryAddr = DAG.getNode(ISD::ADD, dl, PTy, ScaledIndex, Table); + SDValue JumpTarget; + if (isPIC) { + JumpTarget = DAG.getLoad((MVT)MVT::i32, dl, Chain, TabEntryAddr, NULL, 0); + Chain = JumpTarget.getValue(1); + // the table entry is not the actual target but relative to other stuff + // TODO: explain the exact magic here + JumpTarget = DAG.getNode(ISD::ADD, dl, PTy, JumpTarget, Table); + } else { + JumpTarget = DAG.getLoad(PTy, dl, Chain, TabEntryAddr, NULL, 0); + Chain = JumpTarget.getValue(1); + } + + return DAG.getNode(ARMISD::BR_JT, dl, MVT::Other, Chain, JumpTarget, JTI, UId); +} + +SDValue ARMTargetLowering::LowerBR_JT(SDValue Op, SelectionDAG &DAG) { + if (FlagOutlineJumpTables) { + return LowerBR_JT_OutOfLine(Op, DAG, ++ARMJumpTableIndex, getPointerTy()); + } else { + const bool isPIC = getTargetMachine().getRelocationModel() == Reloc::PIC_; + return LowerBR_JT_Inline(Op, DAG, isPIC, getPointerTy()); + } } static SDValue LowerFP_TO_INT(SDValue Op, SelectionDAG &DAG) { Index: lib/Target/ARM/ARMConstantPoolValue.cpp ==================================================================--- lib/Target/ARM/ARMConstantPoolValue.cpp (revision 74072) +++ lib/Target/ARM/ARMConstantPoolValue.cpp (working copy) @@ -84,12 +84,26 @@ } void ARMConstantPoolValue::print(raw_ostream &O) const { + // NOTE: this is not used for codegeneration, moreover + // some of the logic is replicated in + // EmitMachineConstantPoolValue() + if (GV) O << GV->getName(); else O << S; - if (isNonLazyPointer()) O << "$non_lazy_ptr"; - else if (isStub()) O << "$stub"; + + if (isNonLazyPointer()) { + O << "$non_lazy_ptr"; + } else if (isStub()) { + O << "$stub"; + } else if (isDataSegmentJumpTable()) { + // requires synchronization with ARMAsmPrinter.cpp + O << "$jumptable$" << LabelId; + } else { + assert(isValue() && "unknown CP kind"); + } + if (Modifier) O << "(" << Modifier << ")"; if (PCAdjust != 0) { O << "-(LPC" << LabelId << "+" << (unsigned)PCAdjust;
Bob Wilson
2009-Jun-25 22:17 UTC
[LLVMdev] patch for llc/ARM: added mechanism to move switch tables from .text -> .data; also cleanup and documentation
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>
robert muth
2009-Jul-02 17:48 UTC
[LLVMdev] patch for llc/ARM: added mechanism to move switch tables from .text -> .data; also cleanup and documentation
On Thu, Jun 25, 2009 at 6:17 PM, Bob Wilson <bob.wilson at apple.com> wrote:> 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? >I spend over a day trying to follow your suggestion. In the end I was not successful. Here is what Iearned: After setting ARMJITInfo::hasCustomJumpTables -> true setOperationAction for ISD::BR_JT -> Expand I needed to add a "brind" definition to ARMInstrInfo.td I picked "bx" but to do a proper job one would have to take older architectures into account.. The next thing was to to set setOperationAction for ISD::JumpTable -> Custom and implement the corresponding code. This code is very similar to my "outline" code as it has to materialize the table start address. I never quite got this to work, though. Presumably what the generic code generator 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. And it is offset by having to touch ARMInstrInfo.td. Also note, that the generic code generator must make some assumptions about the format of each jumptable entry (e.g. 32bit absolute addresses) so this cannot be changed easily afterwards. Having the default jumtables and the out of line version diverge too much and have different code path may also not be such a good idea.> 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. >I changed all of these as per your request.> > 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. >I changed that too so the data goes into the default .rodata now. I am not sure where .rodata lives. if it lives in the text *segment* you get better protection but the code wont be PIC if .rodata goes into the data segment it is the other way round.> When you create global symbols for the jump tables, you specify a ".T" > name: > > new ARMConstantPoolValue(".T", Num, ARMCP::CPDataSegmentJumpTable); > > Why ".T"? >T as in Table, I do not care much about the naming. If you have a better proposal I will go with that.> > 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. >* fixed and added artificial line break to keep this from happening again. Happy holidays! Robert> > 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 > > > > _______________________________________________ > 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/20090702/e8d432d5/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: patch.2009-07-01 Type: application/octet-stream Size: 15752 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090702/e8d432d5/attachment.obj>
Apparently Analagous Threads
- [LLVMdev] patch for llc/ARM: added mechanism to move switch tables from .text -> .data; also cleanup and documentation
- [LLVMdev] patch for llc/ARM: added mechanism to move switch tables from .text -> .data; also cleanup and documentation
- [LLVMdev] patch for llc/ARM: added mechanism to move switch tables from .text -> .data; also cleanup and documentation
- [LLVMdev] patch for llc/ARM: added mechanism to move switch tables from .text -> .data; also cleanup and documentation
- [LLVMdev] patch for llc/ARM: added mechanism to move switch tables from .text -> .data; also cleanup and documentation