[Mesa-dev] [PATCH 4/7] radeon/llvm: use llvm fabs intrinsic

[Tom Stellard]

On Tue, Oct 09, 2012 at 11:35:00PM +0200, Vincent wrote:
> Le mardi 09 octobre 2012 à 10:49 -0400, Tom Stellard a écrit :
> > On Mon, Oct 08, 2012 at 04:47:10PM +0200, Vincent Lejeune wrote:
> > > ---
> > >  src/gallium/drivers/radeon/AMDGPUISelLowering.cpp   | 3 +--
> > >  src/gallium/drivers/radeon/AMDILIntrinsics.td       | 1 -
> > >  src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c | 6 +++---
> > >  3 files changed, 4 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/src/gallium/drivers/radeon/AMDGPUISelLowering.cpp b/src/gallium/drivers/radeon/AMDGPUISelLowering.cpp
> > > index aee625d..30dd8f3 100644
> > > --- a/src/gallium/drivers/radeon/AMDGPUISelLowering.cpp
> > > +++ b/src/gallium/drivers/radeon/AMDGPUISelLowering.cpp
> > > @@ -36,6 +36,7 @@ AMDGPUTargetLowering::AMDGPUTargetLowering(TargetMachine &TM) :
> > >    setOperationAction(ISD::FEXP2,  MVT::f32, Legal);
> > >    setOperationAction(ISD::FPOW,   MVT::f32, Legal);
> > >    setOperationAction(ISD::FLOG2,  MVT::f32, Legal);
> > > +  setOperationAction(ISD::FABS,   MVT::f32, Legal);
> > >    setOperationAction(ISD::FRINT,  MVT::f32, Legal);
> > >  
> > >    setOperationAction(ISD::UDIV, MVT::i32, Expand);
> > > @@ -110,8 +111,6 @@ SDValue AMDGPUTargetLowering::LowerINTRINSIC_WO_CHAIN(SDValue Op,
> > >        return LowerIntrinsicIABS(Op, DAG);
> > >      case AMDGPUIntrinsic::AMDIL_exp:
> > >        return DAG.getNode(ISD::FEXP2, DL, VT, Op.getOperand(1));
> > > -    case AMDGPUIntrinsic::AMDIL_fabs:
> > > -      return DAG.getNode(ISD::FABS, DL, VT, Op.getOperand(1));
> > >      case AMDGPUIntrinsic::AMDGPU_lrp:
> > >        return LowerIntrinsicLRP(Op, DAG);
> > >      case AMDGPUIntrinsic::AMDIL_fraction:
> > > diff --git a/src/gallium/drivers/radeon/AMDILIntrinsics.td b/src/gallium/drivers/radeon/AMDILIntrinsics.td
> > > index 4de5767..213c8bb 100644
> > > --- a/src/gallium/drivers/radeon/AMDILIntrinsics.td
> > > +++ b/src/gallium/drivers/radeon/AMDILIntrinsics.td
> > > @@ -66,7 +66,6 @@ let TargetPrefix = "AMDIL", isTarget = 1 in {
> > >  }
> > >  
> > >  let TargetPrefix = "AMDIL", isTarget = 1 in {
> > > -  def int_AMDIL_fabs : GCCBuiltin<"__amdil_fabs">, UnaryIntFloat;
> > >    def int_AMDIL_abs : GCCBuiltin<"__amdil_abs">, UnaryIntInt;
> > >  
> > >    def int_AMDIL_bit_extract_i32 : GCCBuiltin<"__amdil_ibit_extract">,
> > > diff --git a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c
> > > index cc690c0..a8327ac 100644
> > > --- a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c
> > > +++ b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c
> > > @@ -538,7 +538,7 @@ static void emit_prepare_cube_coords(
> > >  		coords[i] = LLVMBuildExtractElement(builder, v, idx, "");
> > >  	}
> > >  
> > > -	coords[2] = build_intrinsic(builder, "llvm.AMDIL.fabs.",
> > > +	coords[2] = build_intrinsic(builder, "fabs",
> > >  			type, &coords[2], 1, LLVMReadNoneAttribute);
> > >  	coords[2] = build_intrinsic(builder, "llvm.AMDGPU.rcp",
> > >  			type, &coords[2], 1, LLVMReadNoneAttribute);
> > > @@ -1122,8 +1122,8 @@ void radeon_llvm_context_init(struct radeon_llvm_context * ctx)
> > >  
> > >  
> > >  
> > > -	bld_base->op_actions[TGSI_OPCODE_ABS].emit = build_tgsi_intrinsic_nomem;
> > > -	bld_base->op_actions[TGSI_OPCODE_ABS].intr_name = "llvm.AMDIL.fabs.";
> > > +	bld_base->op_actions[TGSI_OPCODE_ABS].emit = build_tgsi_intrinsic_readonly;
> > > +	bld_base->op_actions[TGSI_OPCODE_ABS].intr_name = "fabs";
> > 
> > I've noticed this on a few of the patches.  Why fabs for the intrinsic
> > name instead of llvm.fabs?
> 
> 
> I didn't find "llvm.fabs" intrinsics ID in neither llvm 3.0 nor llvm
> 3.1, in the include/llvm/Intrinsics.{h,gen} files ; and radeon loader
> always lower call to llvm.fabs/llvm.fabs.f32 to a function call.
> It seems the intrinsic has been superseded by the "fabs" LibFunc symbol
> in llvm. At the instruction selection stage, the fabs function call can
> be lowered to the FABS Node if the target support it, otherwise it uses
> a function call. In fact I noticed that this behavior is shared by a lot
> of function from the libm library (floor, trunc, rint, ...) : there is
> no llvm intrinsic, but the ISD node is generated when a call to the libm
> function is found. (in
> lib/Codegen/SelectionDAG/SelectionDAGBuilder.cpp).
> Some libm function have still an llvm intrinsic like cos, sin, exp, log
> and sqrt. IMHO this allow target support several implementations of
> these functions, a native one and one that uses libm code.
> 
> Regards,
> Vincent
> 
>

Thanks for the explanation.  I just had one small comment on patch 7,
but otherwise, this series is:

Reviewed-by: Tom Stellard <thomas.stellard at amd.com>


> > 
> > -Tom
> > 
> > >  	bld_base->op_actions[TGSI_OPCODE_ARL].emit = build_tgsi_intrinsic_nomem;
> > >  	bld_base->op_actions[TGSI_OPCODE_ARL].intr_name = "llvm.AMDGPU.arl";
> > >  	bld_base->op_actions[TGSI_OPCODE_BGNLOOP].emit = bgnloop_emit;
> > > -- 
> > > 1.7.11.4
> > > 
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>