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

[Jim Grosbach]

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