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.stell...@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@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
