Re: [Mesa-dev] [PATCH] gallivm: use fallback code for mul_hi with llvm >= 7.0
Am 16.10.19 um 01:20 schrieb Dave Airlie: > On Fri, 30 Aug 2019 at 00:55, Roland Scheidegger wrote: >> >> Am 29.08.19 um 15:05 schrieb Jose Fonseca: >>> This change is >>> >>> Reviewed-by: Jose Fonseca >>> >>> Regarding follow up change, do you think the LLVM pattern is sane/doable? >> Yes, should be doable and not too bad (I did not verify that what we're >> doing doesn't actually get recognized, since it's theoretically possible >> some other lowering could produce the pattern, although it seems unlikely). >> I think though this code isn't hit a lot - it was once used by draw, >> which is why I noticed the suboptimal code generated and added the >> optimized version, but nowadays it's just for mulhi, so should be fairly >> rare in practice. >> >> >>> >>> If not we should try ask them to reconsider relying strictly upon >>> pattern matching. I get the feeling upstream LLVM is throwing the baby >>> with the water with these changes. I do understand the advantages of >>> moving away from vendor specific intrinsics, but I think that for things >>> which have no natural representation on LLVM base IR, they should add a >>> vendor-agnostic intrinsic, for example a new "/llvm.mulhi.*" set of >>> instrinsics/, as narrow pattern matching is bound to produce performance >>> cliffs nobody will notice. >> They did add new generic intrinsics for some things, but not this one >> indeed. >> I'm not exactly a big fan of this pattern matching in favor of >> intrinsics neither, at least if the patterns are non-trivial... > > Btw In working on something else, I found the padd and psub intrinsic > paths seem to fail at least on LLVM 8. You mean the signed sse2/avx2 intrinsics for psub/padd? At a quick glance it seems like llvm 8 actually already removed both the signed and unsigned intrinsics in the end (initially only the unsigned versions were removed, which is a lot more noticeable as it is hit basically in all tests). I think the llvm specific intrinsics should work in llvm 8 too. I'll test this... Roland > > I don't have an example test to show though. > > Dave. > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] gallivm: use fallback code for mul_hi with llvm >= 7.0
On Fri, 30 Aug 2019 at 00:55, Roland Scheidegger wrote: > > Am 29.08.19 um 15:05 schrieb Jose Fonseca: > > This change is > > > > Reviewed-by: Jose Fonseca > > > > Regarding follow up change, do you think the LLVM pattern is sane/doable? > Yes, should be doable and not too bad (I did not verify that what we're > doing doesn't actually get recognized, since it's theoretically possible > some other lowering could produce the pattern, although it seems unlikely). > I think though this code isn't hit a lot - it was once used by draw, > which is why I noticed the suboptimal code generated and added the > optimized version, but nowadays it's just for mulhi, so should be fairly > rare in practice. > > > > > > If not we should try ask them to reconsider relying strictly upon > > pattern matching. I get the feeling upstream LLVM is throwing the baby > > with the water with these changes. I do understand the advantages of > > moving away from vendor specific intrinsics, but I think that for things > > which have no natural representation on LLVM base IR, they should add a > > vendor-agnostic intrinsic, for example a new "/llvm.mulhi.*" set of > > instrinsics/, as narrow pattern matching is bound to produce performance > > cliffs nobody will notice. > They did add new generic intrinsics for some things, but not this one > indeed. > I'm not exactly a big fan of this pattern matching in favor of > intrinsics neither, at least if the patterns are non-trivial... Btw In working on something else, I found the padd and psub intrinsic paths seem to fail at least on LLVM 8. I don't have an example test to show though. Dave. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] gallivm: use fallback code for mul_hi with llvm >= 7.0
Am 29.08.19 um 15:05 schrieb Jose Fonseca:
> This change is
>
> Reviewed-by: Jose Fonseca
>
> Regarding follow up change, do you think the LLVM pattern is sane/doable?
Yes, should be doable and not too bad (I did not verify that what we're
doing doesn't actually get recognized, since it's theoretically possible
some other lowering could produce the pattern, although it seems unlikely).
I think though this code isn't hit a lot - it was once used by draw,
which is why I noticed the suboptimal code generated and added the
optimized version, but nowadays it's just for mulhi, so should be fairly
rare in practice.
>
> If not we should try ask them to reconsider relying strictly upon
> pattern matching. I get the feeling upstream LLVM is throwing the baby
> with the water with these changes. I do understand the advantages of
> moving away from vendor specific intrinsics, but I think that for things
> which have no natural representation on LLVM base IR, they should add a
> vendor-agnostic intrinsic, for example a new "/llvm.mulhi.*" set of
> instrinsics/, as narrow pattern matching is bound to produce performance
> cliffs nobody will notice.
They did add new generic intrinsics for some things, but not this one
indeed.
I'm not exactly a big fan of this pattern matching in favor of
intrinsics neither, at least if the patterns are non-trivial...
Roland
> /
> /
> /Jose/
>
>
> *From:* srol...@vmware.com
> *Sent:* Wednesday, August 28, 2019 20:37
> *To:* mesa-dev@lists.freedesktop.org ;
> Jose Fonseca ; airl...@freedesktop.org
>
> *Cc:* Roland Scheidegger
> *Subject:* [PATCH] gallivm: use fallback code for mul_hi with llvm >= 7.0
>
> From: Roland Scheidegger
>
> LLVM 7.0 ditched the pmulu intrinsics.
> This is only a trivial patch to use the fallback code instead.
> It'll likely produce atrocious code since the pattern doesn't match what
> llvm itself uses in its autoupgrade paths, hence the pattern won't be
> recognized.
>
> Should fix https://bugs.freedesktop.org/show_bug.cgi?id=111496
> ---
> src/gallium/auxiliary/gallivm/lp_bld_arit.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_arit.c
> b/src/gallium/auxiliary/gallivm/lp_bld_arit.c
> index c4931c0b230..f1866c6625f 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_arit.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_arit.c
> @@ -1169,8 +1169,13 @@ lp_build_mul_32_lohi_cpu(struct lp_build_context
> *bld,
> * https://llvm.org/bugs/show_bug.cgi?id=30845
> * So, whip up our own code, albeit only for length 4 and 8 (which
> * should be good enough)...
> + * FIXME: For llvm >= 7.0 we should match the autoupgrade pattern
> + * (bitcast/and/mul/shuffle for unsigned, bitcast/shl/ashr/mul/shuffle
> + * for signed), which the fallback code does not, without this llvm
> + * will likely still produce atrocious code.
> */
> - if ((bld->type.length == 4 || bld->type.length == 8) &&
> + if (HAVE_LLVM < 0x0700 &&
> + (bld->type.length == 4 || bld->type.length == 8) &&
> ((util_cpu_caps.has_sse2 && (bld->type.sign == 0)) ||
> util_cpu_caps.has_sse4_1)) {
> const char *intrinsic = NULL;
> --
> 2.17.1
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] gallivm: use fallback code for mul_hi with llvm >= 7.0
This change is
Reviewed-by: Jose Fonseca
Regarding follow up change, do you think the LLVM pattern is sane/doable?
If not we should try ask them to reconsider relying strictly upon pattern
matching. I get the feeling upstream LLVM is throwing the baby with the water
with these changes. I do understand the advantages of moving away from vendor
specific intrinsics, but I think that for things which have no natural
representation on LLVM base IR, they should add a vendor-agnostic intrinsic,
for example a new "llvm.mulhi.*" set of instrinsics, as narrow pattern
matching is bound to produce performance cliffs nobody will notice.
Jose
From: srol...@vmware.com
Sent: Wednesday, August 28, 2019 20:37
To: mesa-dev@lists.freedesktop.org ; Jose
Fonseca ; airl...@freedesktop.org
Cc: Roland Scheidegger
Subject: [PATCH] gallivm: use fallback code for mul_hi with llvm >= 7.0
From: Roland Scheidegger
LLVM 7.0 ditched the pmulu intrinsics.
This is only a trivial patch to use the fallback code instead.
It'll likely produce atrocious code since the pattern doesn't match what
llvm itself uses in its autoupgrade paths, hence the pattern won't be
recognized.
Should fix https://bugs.freedesktop.org/show_bug.cgi?id=111496
---
src/gallium/auxiliary/gallivm/lp_bld_arit.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/gallium/auxiliary/gallivm/lp_bld_arit.c
b/src/gallium/auxiliary/gallivm/lp_bld_arit.c
index c4931c0b230..f1866c6625f 100644
--- a/src/gallium/auxiliary/gallivm/lp_bld_arit.c
+++ b/src/gallium/auxiliary/gallivm/lp_bld_arit.c
@@ -1169,8 +1169,13 @@ lp_build_mul_32_lohi_cpu(struct lp_build_context *bld,
* https://llvm.org/bugs/show_bug.cgi?id=30845
* So, whip up our own code, albeit only for length 4 and 8 (which
* should be good enough)...
+* FIXME: For llvm >= 7.0 we should match the autoupgrade pattern
+* (bitcast/and/mul/shuffle for unsigned, bitcast/shl/ashr/mul/shuffle
+* for signed), which the fallback code does not, without this llvm
+* will likely still produce atrocious code.
*/
- if ((bld->type.length == 4 || bld->type.length == 8) &&
+ if (HAVE_LLVM < 0x0700 &&
+ (bld->type.length == 4 || bld->type.length == 8) &&
((util_cpu_caps.has_sse2 && (bld->type.sign == 0)) ||
util_cpu_caps.has_sse4_1)) {
const char *intrinsic = NULL;
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] gallivm: use fallback code for mul_hi with llvm >= 7.0
Reviewed-by: Dave Airlie
On Thu, 29 Aug 2019 at 05:37, wrote:
>
> From: Roland Scheidegger
>
> LLVM 7.0 ditched the pmulu intrinsics.
> This is only a trivial patch to use the fallback code instead.
> It'll likely produce atrocious code since the pattern doesn't match what
> llvm itself uses in its autoupgrade paths, hence the pattern won't be
> recognized.
>
> Should fix https://bugs.freedesktop.org/show_bug.cgi?id=111496
> ---
> src/gallium/auxiliary/gallivm/lp_bld_arit.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_arit.c
> b/src/gallium/auxiliary/gallivm/lp_bld_arit.c
> index c4931c0b230..f1866c6625f 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_arit.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_arit.c
> @@ -1169,8 +1169,13 @@ lp_build_mul_32_lohi_cpu(struct lp_build_context *bld,
> * https://llvm.org/bugs/show_bug.cgi?id=30845
> * So, whip up our own code, albeit only for length 4 and 8 (which
> * should be good enough)...
> +* FIXME: For llvm >= 7.0 we should match the autoupgrade pattern
> +* (bitcast/and/mul/shuffle for unsigned, bitcast/shl/ashr/mul/shuffle
> +* for signed), which the fallback code does not, without this llvm
> +* will likely still produce atrocious code.
> */
> - if ((bld->type.length == 4 || bld->type.length == 8) &&
> + if (HAVE_LLVM < 0x0700 &&
> + (bld->type.length == 4 || bld->type.length == 8) &&
> ((util_cpu_caps.has_sse2 && (bld->type.sign == 0)) ||
> util_cpu_caps.has_sse4_1)) {
>const char *intrinsic = NULL;
> --
> 2.17.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] gallivm: use fallback code for mul_hi with llvm >= 7.0
From: Roland Scheidegger
LLVM 7.0 ditched the pmulu intrinsics.
This is only a trivial patch to use the fallback code instead.
It'll likely produce atrocious code since the pattern doesn't match what
llvm itself uses in its autoupgrade paths, hence the pattern won't be
recognized.
Should fix https://bugs.freedesktop.org/show_bug.cgi?id=111496
---
src/gallium/auxiliary/gallivm/lp_bld_arit.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/gallium/auxiliary/gallivm/lp_bld_arit.c
b/src/gallium/auxiliary/gallivm/lp_bld_arit.c
index c4931c0b230..f1866c6625f 100644
--- a/src/gallium/auxiliary/gallivm/lp_bld_arit.c
+++ b/src/gallium/auxiliary/gallivm/lp_bld_arit.c
@@ -1169,8 +1169,13 @@ lp_build_mul_32_lohi_cpu(struct lp_build_context *bld,
* https://llvm.org/bugs/show_bug.cgi?id=30845
* So, whip up our own code, albeit only for length 4 and 8 (which
* should be good enough)...
+* FIXME: For llvm >= 7.0 we should match the autoupgrade pattern
+* (bitcast/and/mul/shuffle for unsigned, bitcast/shl/ashr/mul/shuffle
+* for signed), which the fallback code does not, without this llvm
+* will likely still produce atrocious code.
*/
- if ((bld->type.length == 4 || bld->type.length == 8) &&
+ if (HAVE_LLVM < 0x0700 &&
+ (bld->type.length == 4 || bld->type.length == 8) &&
((util_cpu_caps.has_sse2 && (bld->type.sign == 0)) ||
util_cpu_caps.has_sse4_1)) {
const char *intrinsic = NULL;
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
