[llvm-commits] Provide interface in TargetLowering to control jump table.

[Jim Grosbach]

LGTM. Thanks!

-j
On Sep 24, 2012, at 2:52 PM, Ron Lieberman <ronl at codeaurora.org> wrote:

> Jim, Sebastian
> Thanks for the suggestions.
> I have attached a newer patch incorporating your suggestions.
> Please let me know if there are any other issues.
> 
> Ron
> ---
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
> The Linux Foundation
> 
> -----Original Message-----
> From: Jim Grosbach [mailto:grosbach at apple.com] 
> Sent: Monday, September 24, 2012 3:07 PM
> To: Sebastian Pop
> Cc: Ron Lieberman; llvm-commits at cs.uiuc.edu for LLVM
> Subject: Re: [llvm-commits] Provide interface in TargetLowering to control
> jump table.
> 
> 
> On Sep 24, 2012, at 12:53 PM, Sebastian Pop <spop at codeaurora.org> wrote:
> 
>> Hi Jim,
>> 
>> On Mon, Sep 24, 2012 at 2:47 PM, Jim Grosbach <grosbach at apple.com> wrote:
>>> I don't have any conceptual problem with the patch (seems like a good 
>>> idea), but the naming could use a bit of cleaning up. Specifically, see:
>>> http://llvm.org/docs/CodingStandards.html#the-low-level-issues
>> 
>> I am not sure what names introduced by Ron's patch do not follow the 
>> rules in the coding standards: could you please point out which ones?
> 
> Sure thing. The name of the getter method and the TLI variable in
> particular. 
> 
> 
>> From 31f2b6518a2db189490acb389ba1881962d311e4 Mon Sep 17 00:00:00 2001
>> From: Ron Lieberman <ronl at codeaurora.org>
>> Date: Fri, 21 Sep 2012 12:02:38 -0500
>> Subject: [PATCH] Provide interface in TargetLowering to control jump 
>> table  cutover for switches.
>> 
>>    cutoverJumpTables() defaults to 4.
>>    Default is set to 5 for hexagon target.
>> ---
>> include/llvm/Target/TargetLowering.h             |   15 +++++++++++++++
>> lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp |    5 +++--
>> lib/CodeGen/SelectionDAG/TargetLowering.cpp      |    1 +
>> lib/Target/Hexagon/HexagonISelLowering.cpp       |    2 ++
>> 4 files changed, 21 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/llvm/Target/TargetLowering.h 
>> b/include/llvm/Target/TargetLowering.h
>> index ff51048..1673610 100644
>> --- a/include/llvm/Target/TargetLowering.h
>> +++ b/include/llvm/Target/TargetLowering.h
>> @@ -712,6 +712,12 @@ public:
>>     return SupportJumpTables;
>>   }
>> 
>> +  /// cutoverJumpTables - return integer threshold on number of 
>> + blocks to  /// use jump tables rather than if sequence.
>> +  int cutoverJumpTables() const {
>> +    return CutoverJumpTables;
>> +  }
> 
> This is a getter method and should be named as such (i.e., start with
> "get").
> 
>> +
>>   /// getStackPointerRegisterToSaveRestore - If a physical register, this
>>   /// specifies the register that llvm.savestack/llvm.restorestack should
> save
>>   /// and restore.
>> @@ -1032,6 +1038,12 @@ protected:
>>     SupportJumpTables = Val;
>>   }
>> 
>> +  /// setCutoverJumpTables - Indicate the number of blocks to cutover 
>> + to  /// jump tables.
>> +  void setCutoverJumpTables(int Val) {
>> +    CutoverJumpTables = Val;
>> +  }
>> +
>>   /// setStackPointerRegisterToSaveRestore - If set to a physical
> register, this
>>   /// specifies the register that llvm.savestack/llvm.restorestack should
> save
>>   /// and restore.
>> @@ -1826,6 +1838,9 @@ private:
>>   /// If it's not true, then each jumptable must be lowered into
> if-then-else's.
>>   bool SupportJumpTables;
>> 
>> +  /// CutoverJumpTables - Number of blocks threshld for using jump
> tables.
> 
> s/threshld/threshold/
> 
>> +  int CutoverJumpTables;
>> +
> 
> It's not clear from the name if this is a verb "cutover to jump tables!", a
> threshold, or something else. It's clear in the context of the rest of the
> patch, but it'd be better if the name itself were clearer. Something like
> "UseJumpTableThreshold" or "MinimumJumpTableEntries" perhaps? That's off the
> top of my head and could almost certainly be improved, but hopefully gives
> an idea of what I'm on about. Specifically, even knowing what this patch
> does, the name as chosen was confusing to me. I had to read things a couple
> of times to figure out what this actually represents.
> 
> 
>>   /// BooleanContents - Information about the contents of the high-bits
> in
>>   /// boolean values held in a type wider than i1.  See
> getBooleanContents.
>>   BooleanContent BooleanContents;
>> diff --git a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp 
>> b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
>> index 483b051..e81a3b9 100644
>> --- a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
>> +++ b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
>> @@ -2093,7 +2093,7 @@ bool SelectionDAGBuilder::handleJTSwitchCase(CaseRec
> &CR,
>>   for (CaseItr I = CR.Range.first, E = CR.Range.second; I != E; ++I)
>>     TSize += I->size();
>> 
>> -  if (!areJTsAllowed(TLI) || TSize.ult(4))
>> +  if (!areJTsAllowed(TLI) || TSize.ult(TLI.cutoverJumpTables()))
>>     return false;
>> 
>>   APInt Range = ComputeRange(First, Last); @@ -2565,9 +2565,10 @@ 
>> void SelectionDAGBuilder::visitSwitch(const SwitchInst &SI) {
>>     if (handleSmallSwitchRange(CR, WorkList, SV, Default, SwitchMBB))
>>       continue;
>> 
>> -    // If the switch has more than 5 blocks, and at least 40% dense, and
> the
>> +    // If the switch has more than N blocks, and is at least 40% 
>> + dense, and the
>>     // target supports indirect branches, then emit a jump table rather
> than
>>     // lowering the switch to a binary tree of conditional branches.
>> +    // N defaults to 4 and is controlled via TLS.cutoverJumpTables().
>>     if (handleJTSwitchCase(CR, WorkList, SV, Default, SwitchMBB))
>>       continue;
>> 
>> diff --git a/lib/CodeGen/SelectionDAG/TargetLowering.cpp 
>> b/lib/CodeGen/SelectionDAG/TargetLowering.cpp
>> index 56f3a45..10d4727 100644
>> --- a/lib/CodeGen/SelectionDAG/TargetLowering.cpp
>> +++ b/lib/CodeGen/SelectionDAG/TargetLowering.cpp
>> @@ -613,6 +613,7 @@ TargetLowering::TargetLowering(const TargetMachine
> &tm,
>>   ShouldFoldAtomicFences = false;
>>   InsertFencesForAtomic = false;
>>   SupportJumpTables = true;
>> +  CutoverJumpTables = 4;
>> 
>>   InitLibcallNames(LibcallRoutineNames);
>>   InitCmpLibcallCCs(CmpLibcallCCs);
>> diff --git a/lib/Target/Hexagon/HexagonISelLowering.cpp 
>> b/lib/Target/Hexagon/HexagonISelLowering.cpp
>> index 703a128..78d56d2 100644
>> --- a/lib/Target/Hexagon/HexagonISelLowering.cpp
>> +++ b/lib/Target/Hexagon/HexagonISelLowering.cpp
>> @@ -1350,6 +1350,8 @@
> HexagonTargetLowering::HexagonTargetLowering(HexagonTargetMachine
>>     } else {
>>       setOperationAction(ISD::BR_JT, MVT::Other, Expand);
>>     }
>> +    // Increase jump tables cutover to 5, was 4.
> 
> This should be more explicit about what that means. That is, why this is
> being done not just what it's doing. Something along the lines of, "The
> default switch table formation has a minimum threshold of at least four
> basic block targets in the switch. For Hexxagon, this threshold is five
> because[.]".
> 
>> +    setCutoverJumpTables(5);
>> 
>>     setOperationAction(ISD::BR_CC, MVT::i32, Expand);
>> 
>> --
>> 1.7.6.4
>> 
> 
>> 
>> Thanks,
>> Sebastian
>> -- 
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> hosted by The Linux Foundation
> 
> <0001-TargetLowering-interface-to-set-get-minimum-block-en.patch>