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

[Ron Lieberman]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-TargetLowering-interface-to-set-get-minimum-block-en.patch
Type: application/octet-stream
Size: 4871 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120924/96588fa5/attachment.obj>