Re: [Mesa-dev] [PATCH] radeon/llvm: Use llvm.AMDIL.exp intrinsic again for now
On 20.11.2015 03:22, Michel Dänzer wrote: On 20.11.2015 02:11, Nicolai Hähnle wrote: On 19.11.2015 17:37, Tom Stellard wrote: On Thu, Nov 19, 2015 at 11:17:12AM +0100, Nicolai Hähnle wrote: On 19.11.2015 03:55, Tom Stellard wrote: On Thu, Nov 19, 2015 at 11:31:55AM +0900, Michel Dänzer wrote: From: Michel Dänzer llvm.exp2.f32 doesn't work in some cases yet. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92709 Signed-off-by: Michel Dänzer --- Once the problem is fixed in the LLVM AMDGPU backend, we can re-enable llvm.exp2.f32 for the fixed LLVM version. src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c index ac99e73..c94f109 100644 --- a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c +++ b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c @@ -1539,7 +1539,7 @@ void radeon_llvm_context_init(struct radeon_llvm_context * ctx) bld_base->op_actions[TGSI_OPCODE_ENDIF].emit = endif_emit; bld_base->op_actions[TGSI_OPCODE_ENDLOOP].emit = endloop_emit; bld_base->op_actions[TGSI_OPCODE_EX2].emit = build_tgsi_intrinsic_nomem; -bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = "llvm.exp2.f32"; +bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = "llvm.AMDIL.exp."; Do we want a native instruction here, or do we want IEEE precise exp2? If it's the former then we shouldn't be using llvm.exp2.f32 anyway. I know that we need to use llvm.AMDIL.exp. for older LLVM, but for newer LLVM, I would really like to start doing intrinsics the correct way. In this case, it means adding an llvm.amdgcn.exp.f32 intrinsic to include/llvm/IR/IntrinsicsAMDGPU.td. In the section with the amdgcn TargetPrefix. Doesn't AMDGPU currently emit native instructions for the various math intrinsics anyway? If your plan is to change that and we add our own intrinsics, what's the plan to still benefit from existing LLVM optimization passes? AMDGPU does emit native instruction for the math intrinsics, but this is wrong in most cases, because it assumes that a less precise calculation is acceptable when this is not always the case. Okay, that makes sense. This a good point about optimizations. I think the main benefits we get from using the LLVM intrinsic are constant folding and also speculative execution. An alternate solution would be to use the llvm intrinsic, but then communicate to the backend via a function attribute that it is OK to use the less precise version of exp. I like that alternate solution. FWIW, the original bug that Michel addresses here is caused by LLVM libcall optimizations replacing the exp2 _intrinsic_ by a ldexp _libcall_, which AMDGPU codegen cannot deal with. I suggested to address this with http://reviews.llvm.org/D14327. Could you take a look at that? Please take a look at the comment I added to this review. Done :) Meanwhile, can I get an R-b for this patch to fix the immediate regression in Mesa Git until LLVM is fixed (and for released versions of LLVM)? Right, sorry about that: Reviewed-by: Nicolai Hähnle ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] radeon/llvm: Use llvm.AMDIL.exp intrinsic again for now
On 20.11.2015 02:11, Nicolai Hähnle wrote: > On 19.11.2015 17:37, Tom Stellard wrote: >> On Thu, Nov 19, 2015 at 11:17:12AM +0100, Nicolai Hähnle wrote: >>> On 19.11.2015 03:55, Tom Stellard wrote: On Thu, Nov 19, 2015 at 11:31:55AM +0900, Michel Dänzer wrote: > From: Michel Dänzer > > llvm.exp2.f32 doesn't work in some cases yet. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92709 > Signed-off-by: Michel Dänzer > --- > > Once the problem is fixed in the LLVM AMDGPU backend, we can re-enable > llvm.exp2.f32 for the fixed LLVM version. > > src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > index ac99e73..c94f109 100644 > --- a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > +++ b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > @@ -1539,7 +1539,7 @@ void radeon_llvm_context_init(struct > radeon_llvm_context * ctx) > bld_base->op_actions[TGSI_OPCODE_ENDIF].emit = endif_emit; > bld_base->op_actions[TGSI_OPCODE_ENDLOOP].emit = endloop_emit; > bld_base->op_actions[TGSI_OPCODE_EX2].emit = > build_tgsi_intrinsic_nomem; > -bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = > "llvm.exp2.f32"; > +bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = > "llvm.AMDIL.exp."; Do we want a native instruction here, or do we want IEEE precise exp2? If it's the former then we shouldn't be using llvm.exp2.f32 anyway. I know that we need to use llvm.AMDIL.exp. for older LLVM, but for newer LLVM, I would really like to start doing intrinsics the correct way. In this case, it means adding an llvm.amdgcn.exp.f32 intrinsic to include/llvm/IR/IntrinsicsAMDGPU.td. In the section with the amdgcn TargetPrefix. >>> >>> Doesn't AMDGPU currently emit native instructions for the various >>> math intrinsics anyway? If your plan is to change that and we add >>> our own intrinsics, what's the plan to still benefit from existing >>> LLVM optimization passes? >>> >> >> AMDGPU does emit native instruction for the math intrinsics, but this is >> wrong in most cases, because it assumes that a less precise >> calculation is >> acceptable when this is not always the case. > > Okay, that makes sense. > >> This a good point about optimizations. I think the main benefits we >> get from >> using the LLVM intrinsic are constant folding and also speculative >> execution. >> >> An alternate solution would be to use the llvm intrinsic, but then >> communicate >> to the backend via a function attribute that it is OK to use the less >> precise >> version of exp. > > I like that alternate solution. > >>> FWIW, the original bug that Michel addresses here is caused by LLVM >>> libcall optimizations replacing the exp2 _intrinsic_ by a ldexp >>> _libcall_, which AMDGPU codegen cannot deal with. I suggested to >>> address this with http://reviews.llvm.org/D14327. Could you take a >>> look at that? >>> >> >> Please take a look at the comment I added to this review. > > Done :) Meanwhile, can I get an R-b for this patch to fix the immediate regression in Mesa Git until LLVM is fixed (and for released versions of LLVM)? -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] radeon/llvm: Use llvm.AMDIL.exp intrinsic again for now
On 19.11.2015 17:37, Tom Stellard wrote: On Thu, Nov 19, 2015 at 11:17:12AM +0100, Nicolai Hähnle wrote: On 19.11.2015 03:55, Tom Stellard wrote: On Thu, Nov 19, 2015 at 11:31:55AM +0900, Michel Dänzer wrote: From: Michel Dänzer llvm.exp2.f32 doesn't work in some cases yet. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92709 Signed-off-by: Michel Dänzer --- Once the problem is fixed in the LLVM AMDGPU backend, we can re-enable llvm.exp2.f32 for the fixed LLVM version. src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c index ac99e73..c94f109 100644 --- a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c +++ b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c @@ -1539,7 +1539,7 @@ void radeon_llvm_context_init(struct radeon_llvm_context * ctx) bld_base->op_actions[TGSI_OPCODE_ENDIF].emit = endif_emit; bld_base->op_actions[TGSI_OPCODE_ENDLOOP].emit = endloop_emit; bld_base->op_actions[TGSI_OPCODE_EX2].emit = build_tgsi_intrinsic_nomem; - bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = "llvm.exp2.f32"; + bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = "llvm.AMDIL.exp."; Do we want a native instruction here, or do we want IEEE precise exp2? If it's the former then we shouldn't be using llvm.exp2.f32 anyway. I know that we need to use llvm.AMDIL.exp. for older LLVM, but for newer LLVM, I would really like to start doing intrinsics the correct way. In this case, it means adding an llvm.amdgcn.exp.f32 intrinsic to include/llvm/IR/IntrinsicsAMDGPU.td. In the section with the amdgcn TargetPrefix. Doesn't AMDGPU currently emit native instructions for the various math intrinsics anyway? If your plan is to change that and we add our own intrinsics, what's the plan to still benefit from existing LLVM optimization passes? AMDGPU does emit native instruction for the math intrinsics, but this is wrong in most cases, because it assumes that a less precise calculation is acceptable when this is not always the case. Okay, that makes sense. This a good point about optimizations. I think the main benefits we get from using the LLVM intrinsic are constant folding and also speculative execution. An alternate solution would be to use the llvm intrinsic, but then communicate to the backend via a function attribute that it is OK to use the less precise version of exp. I like that alternate solution. FWIW, the original bug that Michel addresses here is caused by LLVM libcall optimizations replacing the exp2 _intrinsic_ by a ldexp _libcall_, which AMDGPU codegen cannot deal with. I suggested to address this with http://reviews.llvm.org/D14327. Could you take a look at that? Please take a look at the comment I added to this review. Done :) Cheers, Nicolai -Tom Cheers, Nicolai ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] radeon/llvm: Use llvm.AMDIL.exp intrinsic again for now
On Thu, Nov 19, 2015 at 11:17:12AM +0100, Nicolai Hähnle wrote: > On 19.11.2015 03:55, Tom Stellard wrote: > >On Thu, Nov 19, 2015 at 11:31:55AM +0900, Michel Dänzer wrote: > >>From: Michel Dänzer > >> > >>llvm.exp2.f32 doesn't work in some cases yet. > >> > >>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92709 > >>Signed-off-by: Michel Dänzer > >>--- > >> > >>Once the problem is fixed in the LLVM AMDGPU backend, we can re-enable > >>llvm.exp2.f32 for the fixed LLVM version. > >> > >> src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >>diff --git a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > >>b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > >>index ac99e73..c94f109 100644 > >>--- a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > >>+++ b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > >>@@ -1539,7 +1539,7 @@ void radeon_llvm_context_init(struct > >>radeon_llvm_context * ctx) > >>bld_base->op_actions[TGSI_OPCODE_ENDIF].emit = endif_emit; > >>bld_base->op_actions[TGSI_OPCODE_ENDLOOP].emit = endloop_emit; > >>bld_base->op_actions[TGSI_OPCODE_EX2].emit = build_tgsi_intrinsic_nomem; > >>- bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = "llvm.exp2.f32"; > >>+ bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = "llvm.AMDIL.exp."; > > > >Do we want a native instruction here, or do we want IEEE precise exp2? > >If it's the former then we shouldn't be using llvm.exp2.f32 anyway. > > > >I know that we need to use llvm.AMDIL.exp. for older LLVM, but for newer > >LLVM, I would really like to start doing intrinsics the correct way. In > >this case, it means adding an llvm.amdgcn.exp.f32 intrinsic to > >include/llvm/IR/IntrinsicsAMDGPU.td. In the section with the amdgcn > >TargetPrefix. > > Doesn't AMDGPU currently emit native instructions for the various > math intrinsics anyway? If your plan is to change that and we add > our own intrinsics, what's the plan to still benefit from existing > LLVM optimization passes? > AMDGPU does emit native instruction for the math intrinsics, but this is wrong in most cases, because it assumes that a less precise calculation is acceptable when this is not always the case. This a good point about optimizations. I think the main benefits we get from using the LLVM intrinsic are constant folding and also speculative execution. An alternate solution would be to use the llvm intrinsic, but then communicate to the backend via a function attribute that it is OK to use the less precise version of exp. > FWIW, the original bug that Michel addresses here is caused by LLVM > libcall optimizations replacing the exp2 _intrinsic_ by a ldexp > _libcall_, which AMDGPU codegen cannot deal with. I suggested to > address this with http://reviews.llvm.org/D14327. Could you take a > look at that? > Please take a look at the comment I added to this review. -Tom > Cheers, > Nicolai ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] radeon/llvm: Use llvm.AMDIL.exp intrinsic again for now
On 19.11.2015 03:55, Tom Stellard wrote: On Thu, Nov 19, 2015 at 11:31:55AM +0900, Michel Dänzer wrote: From: Michel Dänzer llvm.exp2.f32 doesn't work in some cases yet. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92709 Signed-off-by: Michel Dänzer --- Once the problem is fixed in the LLVM AMDGPU backend, we can re-enable llvm.exp2.f32 for the fixed LLVM version. src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c index ac99e73..c94f109 100644 --- a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c +++ b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c @@ -1539,7 +1539,7 @@ void radeon_llvm_context_init(struct radeon_llvm_context * ctx) bld_base->op_actions[TGSI_OPCODE_ENDIF].emit = endif_emit; bld_base->op_actions[TGSI_OPCODE_ENDLOOP].emit = endloop_emit; bld_base->op_actions[TGSI_OPCODE_EX2].emit = build_tgsi_intrinsic_nomem; - bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = "llvm.exp2.f32"; + bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = "llvm.AMDIL.exp."; Do we want a native instruction here, or do we want IEEE precise exp2? If it's the former then we shouldn't be using llvm.exp2.f32 anyway. I know that we need to use llvm.AMDIL.exp. for older LLVM, but for newer LLVM, I would really like to start doing intrinsics the correct way. In this case, it means adding an llvm.amdgcn.exp.f32 intrinsic to include/llvm/IR/IntrinsicsAMDGPU.td. In the section with the amdgcn TargetPrefix. Doesn't AMDGPU currently emit native instructions for the various math intrinsics anyway? If your plan is to change that and we add our own intrinsics, what's the plan to still benefit from existing LLVM optimization passes? FWIW, the original bug that Michel addresses here is caused by LLVM libcall optimizations replacing the exp2 _intrinsic_ by a ldexp _libcall_, which AMDGPU codegen cannot deal with. I suggested to address this with http://reviews.llvm.org/D14327. Could you take a look at that? Cheers, Nicolai ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] radeon/llvm: Use llvm.AMDIL.exp intrinsic again for now
On Thu, Nov 19, 2015 at 11:31:55AM +0900, Michel Dänzer wrote: > From: Michel Dänzer > > llvm.exp2.f32 doesn't work in some cases yet. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92709 > Signed-off-by: Michel Dänzer > --- > > Once the problem is fixed in the LLVM AMDGPU backend, we can re-enable > llvm.exp2.f32 for the fixed LLVM version. > > src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > index ac99e73..c94f109 100644 > --- a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > +++ b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > @@ -1539,7 +1539,7 @@ void radeon_llvm_context_init(struct > radeon_llvm_context * ctx) > bld_base->op_actions[TGSI_OPCODE_ENDIF].emit = endif_emit; > bld_base->op_actions[TGSI_OPCODE_ENDLOOP].emit = endloop_emit; > bld_base->op_actions[TGSI_OPCODE_EX2].emit = build_tgsi_intrinsic_nomem; > - bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = "llvm.exp2.f32"; > + bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = "llvm.AMDIL.exp."; Do we want a native instruction here, or do we want IEEE precise exp2? If it's the former then we shouldn't be using llvm.exp2.f32 anyway. I know that we need to use llvm.AMDIL.exp. for older LLVM, but for newer LLVM, I would really like to start doing intrinsics the correct way. In this case, it means adding an llvm.amdgcn.exp.f32 intrinsic to include/llvm/IR/IntrinsicsAMDGPU.td. In the section with the amdgcn TargetPrefix. -Tom > bld_base->op_actions[TGSI_OPCODE_FLR].emit = build_tgsi_intrinsic_nomem; > bld_base->op_actions[TGSI_OPCODE_FLR].intr_name = "llvm.floor.f32"; > bld_base->op_actions[TGSI_OPCODE_FMA].emit = build_tgsi_intrinsic_nomem; > -- > 2.6.2 > > ___ > 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
[Mesa-dev] [PATCH] radeon/llvm: Use llvm.AMDIL.exp intrinsic again for now
From: Michel Dänzer llvm.exp2.f32 doesn't work in some cases yet. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92709 Signed-off-by: Michel Dänzer --- Once the problem is fixed in the LLVM AMDGPU backend, we can re-enable llvm.exp2.f32 for the fixed LLVM version. src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c index ac99e73..c94f109 100644 --- a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c +++ b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c @@ -1539,7 +1539,7 @@ void radeon_llvm_context_init(struct radeon_llvm_context * ctx) bld_base->op_actions[TGSI_OPCODE_ENDIF].emit = endif_emit; bld_base->op_actions[TGSI_OPCODE_ENDLOOP].emit = endloop_emit; bld_base->op_actions[TGSI_OPCODE_EX2].emit = build_tgsi_intrinsic_nomem; - bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = "llvm.exp2.f32"; + bld_base->op_actions[TGSI_OPCODE_EX2].intr_name = "llvm.AMDIL.exp."; bld_base->op_actions[TGSI_OPCODE_FLR].emit = build_tgsi_intrinsic_nomem; bld_base->op_actions[TGSI_OPCODE_FLR].intr_name = "llvm.floor.f32"; bld_base->op_actions[TGSI_OPCODE_FMA].emit = build_tgsi_intrinsic_nomem; -- 2.6.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
