[Mesa-dev] [PATCH] radeon/llvm: add a pattern for min/max

[Tom Stellard]

On Wed, Dec 05, 2012 at 04:58:21PM +0100, Vincent Lejeune wrote:
> ---
>  lib/Target/AMDGPU/AMDGPUISelLowering.cpp | 53 ++++++++++++++++++++++++++++++++
>  lib/Target/AMDGPU/AMDGPUISelLowering.h   |  1 +
>  lib/Target/AMDGPU/R600ISelLowering.cpp   |  5 +++
>  lib/Target/AMDGPU/SIISelLowering.cpp     |  6 ++++
>  4 files changed, 65 insertions(+)
>

Hi Vincent,

This looks much better, thanks.  Just a few minor issues, but with those fixed,
this patch is:

Reviewed-by: Tom Stellard <thomas.stellard at amd.com>
 
> diff --git a/lib/Target/AMDGPU/AMDGPUISelLowering.cpp b/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
> index 5bde9db..4c1a070 100644
> --- a/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
> +++ b/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
> @@ -172,7 +172,60 @@ SDValue AMDGPUTargetLowering::LowerIntrinsicLRP(SDValue Op,
>        OneSubAC);
>  }
>  
> +/// Generate Min/Max pattern
> +SDValue AMDGPUTargetLowering::LowerMinMax(SDValue Op,
> +    SelectionDAG &DAG) const
> +{

Brace needs to be on the same line as the function here

> +  DebugLoc DL = Op.getDebugLoc();
> +  EVT VT = Op.getValueType();
>  
> +  SDValue LHS = Op.getOperand(0);
> +  SDValue RHS = Op.getOperand(1);
> +  SDValue True = Op.getOperand(2);
> +  SDValue CC = Op.getOperand(4);

You should move this check into the function:

EVT CompareVT = LHS.getValueType();

if (CompareVT != VT || VT != MVT::f32 ||
    (LHS != True && LHS != False) ||
    (RHS != True && RHS != False)) {
  return SDValue();
}

Make sure to double check my logic here.

> +  ISD::CondCode CCOpcode = cast<CondCodeSDNode>(CC)->get();
> +  switch (CCOpcode) {
> +  case ISD::SETOEQ:
> +  case ISD::SETONE:
> +  case ISD::SETUNE:
> +  case ISD::SETNE:
> +  case ISD::SETUEQ:
> +  case ISD::SETEQ:
> +  case ISD::SETFALSE:
> +  case ISD::SETFALSE2:
> +  case ISD::SETTRUE:
> +  case ISD::SETTRUE2:
> +  case ISD::SETUO:
> +  case ISD::SETO:
> +    assert(0 && "Operation should already be optimised !");
> +  case ISD::SETULE:
> +  case ISD::SETULT:
> +  case ISD::SETOLE:
> +  case ISD::SETOLT:
> +  case ISD::SETLE:
> +  case ISD::SETLT: {
> +    if (LHS == True)
> +      return DAG.getNode(AMDGPUISD::FMIN, DL, VT, LHS, RHS);
> +    else
> +      return DAG.getNode(AMDGPUISD::FMAX, DL, VT, LHS, RHS);
> +  }
> +  case ISD::SETGT:
> +  case ISD::SETGE:
> +  case ISD::SETUGE:
> +  case ISD::SETOGE:
> +  case ISD::SETUGT:
> +  case ISD::SETOGT: {
> +    if (LHS == True)
> +      return DAG.getNode(AMDGPUISD::FMAX, DL, VT, LHS, RHS);
> +    else
> +      return DAG.getNode(AMDGPUISD::FMIN, DL, VT, LHS, RHS);
> +  }
> +  case ISD::SETCC_INVALID:
> +    assert(0 && "Invalid setcc condcode !");
> +  }
> +  return Op;
> +}
>  
>  SDValue AMDGPUTargetLowering::LowerUDIVREM(SDValue Op,
>      SelectionDAG &DAG) const
> diff --git a/lib/Target/AMDGPU/AMDGPUISelLowering.h b/lib/Target/AMDGPU/AMDGPUISelLowering.h
> index 60de190..3b60ae1 100644
> --- a/lib/Target/AMDGPU/AMDGPUISelLowering.h
> +++ b/lib/Target/AMDGPU/AMDGPUISelLowering.h
> @@ -56,6 +56,7 @@ public:
>    virtual SDValue LowerOperation(SDValue Op, SelectionDAG &DAG) const;
>    SDValue LowerIntrinsicIABS(SDValue Op, SelectionDAG &DAG) const;
>    SDValue LowerIntrinsicLRP(SDValue Op, SelectionDAG &DAG) const;
> +  SDValue LowerMinMax(SDValue Op, SelectionDAG &DAG) const;
>    virtual const char* getTargetNodeName(unsigned Opcode) const;
>  
>  // Functions defined in AMDILISelLowering.cpp
> diff --git a/lib/Target/AMDGPU/R600ISelLowering.cpp b/lib/Target/AMDGPU/R600ISelLowering.cpp
> index 6f1c1d7..374fbfc 100644
> --- a/lib/Target/AMDGPU/R600ISelLowering.cpp
> +++ b/lib/Target/AMDGPU/R600ISelLowering.cpp
> @@ -764,6 +764,11 @@ SDValue R600TargetLowering::LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const
>      }
>    }
>  
> +  if (CompareVT == VT == MVT::f32 &&
> +      ((LHS == True && RHS == False) || (LHS == False && RHS == True))) {
> +    return LowerMinMax(Op, DAG);
> +  }
> +

With this check in the common function, replace the above code with:

SDValue MinMax = LowerMinMax(Op, DAG)
if (MinMax.getNode()) {
  return MinMax;
}

>    // If we make it this for it means we have no native instructions to handle
>    // this SELECT_CC, so we must lower it.
>    SDValue HWTrue, HWFalse;
> diff --git a/lib/Target/AMDGPU/SIISelLowering.cpp b/lib/Target/AMDGPU/SIISelLowering.cpp
> index 45f180f..98a7376 100644
> --- a/lib/Target/AMDGPU/SIISelLowering.cpp
> +++ b/lib/Target/AMDGPU/SIISelLowering.cpp
> @@ -383,6 +383,12 @@ SDValue SITargetLowering::LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const
>    EVT VT = Op.getValueType();
>    DebugLoc DL = Op.getDebugLoc();
>  
> +  EVT CompareVT = LHS.getValueType();
> +  if (CompareVT == VT == MVT::f32 &&
> +      ((LHS == True && RHS == False) || (LHS == False && RHS == True))) {
> +    return LowerMinMax(Op, DAG);
> +  }
> +

You can do the same replacement here too.

>    SDValue Cond = DAG.getNode(ISD::SETCC, DL, MVT::i1, LHS, RHS, CC);
>    return DAG.getNode(ISD::SELECT, DL, VT, Cond, True, False);
>  }
> -- 
> 1.8.0.1
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev