[RFC] gallivm: Use new LLVM fast-math-flags API

Submitted by Tobias Droste on Nov. 6, 2017, 9:09 p.m.

Details

Message ID 20171106210950.3417-1-tdroste@gmx.de
State Accepted
Commit 5d61fa4e68b7eb6d481a37efdbb35fdce675a6ad
Headers show
Series "gallivm: Use new LLVM fast-math-flags API" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Tobias Droste Nov. 6, 2017, 9:09 p.m.
LLVM 6 changed the API on the fast-math-flags:
https://reviews.llvm.org/rL317488

NOTE: This also enables the new flag 'ApproxFunc' to allow for
approximations for library functions (sin, cos, ...). I'm not completly
convinced, that this is something mesa should do.

Signed-off-by: Tobias Droste <tdroste@gmx.de>
---
 src/gallium/auxiliary/gallivm/lp_bld_misc.cpp | 4 ++++
 1 file changed, 4 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
index d988910a7e..1319407290 100644
--- a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
+++ b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
@@ -830,7 +830,11 @@  lp_create_builder(LLVMContextRef ctx, enum lp_float_mode float_mode)
       llvm::unwrap(builder)->setFastMathFlags(flags);
       break;
    case LP_FLOAT_MODE_UNSAFE_FP_MATH:
+#if HAVE_LLVM >= 0x0600
+      flags.setFast();
+#else
       flags.setUnsafeAlgebra();
+#endif
       llvm::unwrap(builder)->setFastMathFlags(flags);
       break;
    }

Comments

Reviewed-by: Marek Olšák <marek.olsak@amd.com>

Marek

On Mon, Nov 6, 2017 at 10:09 PM, Tobias Droste <tdroste@gmx.de> wrote:
> LLVM 6 changed the API on the fast-math-flags:
> https://reviews.llvm.org/rL317488
>
> NOTE: This also enables the new flag 'ApproxFunc' to allow for
> approximations for library functions (sin, cos, ...). I'm not completly
> convinced, that this is something mesa should do.
>
> Signed-off-by: Tobias Droste <tdroste@gmx.de>
> ---
>  src/gallium/auxiliary/gallivm/lp_bld_misc.cpp | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
> index d988910a7e..1319407290 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
> @@ -830,7 +830,11 @@ lp_create_builder(LLVMContextRef ctx, enum lp_float_mode float_mode)
>        llvm::unwrap(builder)->setFastMathFlags(flags);
>        break;
>     case LP_FLOAT_MODE_UNSAFE_FP_MATH:
> +#if HAVE_LLVM >= 0x0600
> +      flags.setFast();
> +#else
>        flags.setUnsafeAlgebra();
> +#endif
>        llvm::unwrap(builder)->setFastMathFlags(flags);
>        break;
>     }
> --
> 2.14.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Hi,

Could this (commit 5d61fa4e68b7eb6d481a37efdbb35fdce675a6ad on master) be
backported to the 17.3 branch to allow it to build with LLVM 6?

Thanks,
Alex

On 6 November 2017 at 21:16, Marek Olšák <maraeo@gmail.com> wrote:

> Reviewed-by: Marek Olšák <marek.olsak@amd.com>
>
> Marek
>
> On Mon, Nov 6, 2017 at 10:09 PM, Tobias Droste <tdroste@gmx.de> wrote:
> > LLVM 6 changed the API on the fast-math-flags:
> > https://reviews.llvm.org/rL317488
> >
> > NOTE: This also enables the new flag 'ApproxFunc' to allow for
> > approximations for library functions (sin, cos, ...). I'm not completly
> > convinced, that this is something mesa should do.
> >
> > Signed-off-by: Tobias Droste <tdroste@gmx.de>
> > ---
> >  src/gallium/auxiliary/gallivm/lp_bld_misc.cpp | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
> b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
> > index d988910a7e..1319407290 100644
> > --- a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
> > +++ b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
> > @@ -830,7 +830,11 @@ lp_create_builder(LLVMContextRef ctx, enum
> lp_float_mode float_mode)
> >        llvm::unwrap(builder)->setFastMathFlags(flags);
> >        break;
> >     case LP_FLOAT_MODE_UNSAFE_FP_MATH:
> > +#if HAVE_LLVM >= 0x0600
> > +      flags.setFast();
> > +#else
> >        flags.setUnsafeAlgebra();
> > +#endif
> >        llvm::unwrap(builder)->setFastMathFlags(flags);
> >        break;
> >     }
> > --
> > 2.14.3
> >
> > _______________________________________________
> > 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
>
Yes.

Marek

On Wed, Feb 28, 2018 at 10:25 AM, Alex Smith
<asmith@feralinteractive.com> wrote:
> Hi,
>
> Could this (commit 5d61fa4e68b7eb6d481a37efdbb35fdce675a6ad on master) be
> backported to the 17.3 branch to allow it to build with LLVM 6?
>
> Thanks,
> Alex
>
> On 6 November 2017 at 21:16, Marek Olšák <maraeo@gmail.com> wrote:
>>
>> Reviewed-by: Marek Olšák <marek.olsak@amd.com>
>>
>> Marek
>>
>> On Mon, Nov 6, 2017 at 10:09 PM, Tobias Droste <tdroste@gmx.de> wrote:
>> > LLVM 6 changed the API on the fast-math-flags:
>> > https://reviews.llvm.org/rL317488
>> >
>> > NOTE: This also enables the new flag 'ApproxFunc' to allow for
>> > approximations for library functions (sin, cos, ...). I'm not completly
>> > convinced, that this is something mesa should do.
>> >
>> > Signed-off-by: Tobias Droste <tdroste@gmx.de>
>> > ---
>> >  src/gallium/auxiliary/gallivm/lp_bld_misc.cpp | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
>> > b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
>> > index d988910a7e..1319407290 100644
>> > --- a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
>> > +++ b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
>> > @@ -830,7 +830,11 @@ lp_create_builder(LLVMContextRef ctx, enum
>> > lp_float_mode float_mode)
>> >        llvm::unwrap(builder)->setFastMathFlags(flags);
>> >        break;
>> >     case LP_FLOAT_MODE_UNSAFE_FP_MATH:
>> > +#if HAVE_LLVM >= 0x0600
>> > +      flags.setFast();
>> > +#else
>> >        flags.setUnsafeAlgebra();
>> > +#endif
>> >        llvm::unwrap(builder)->setFastMathFlags(flags);
>> >        break;
>> >     }
>> > --
>> > 2.14.3
>> >
>> > _______________________________________________
>> > 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
>
>
Hi Alex,

On 28 February 2018 at 15:25, Alex Smith <asmith@feralinteractive.com> wrote:
> Hi,
>
> Could this (commit 5d61fa4e68b7eb6d481a37efdbb35fdce675a6ad on master) be
> backported to the 17.3 branch to allow it to build with LLVM 6?
>
Normally we don't aim to support LLVM versions released after the .0
Mesa release is out.
Not that we don't want to - there is simply not enough testing happening.

Sometimes picking the odd build fix is enough, but not always.

As a matter of fact, the only feedback for the AMD drivers status
(brokenness) is the LunarG testing rig.

Michel, usually you are usually more realistic/conservative on with
this kind of changes.
Any objections?

Thanks
Emil
On Fri, Mar 2, 2018 at 1:38 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> Hi Alex,
>
> On 28 February 2018 at 15:25, Alex Smith <asmith@feralinteractive.com> wrote:
>> Hi,
>>
>> Could this (commit 5d61fa4e68b7eb6d481a37efdbb35fdce675a6ad on master) be
>> backported to the 17.3 branch to allow it to build with LLVM 6?
>>
> Normally we don't aim to support LLVM versions released after the .0
> Mesa release is out.
> Not that we don't want to - there is simply not enough testing happening.
>
> Sometimes picking the odd build fix is enough, but not always.
>
> As a matter of fact, the only feedback for the AMD drivers status
> (brokenness) is the LunarG testing rig.
>
> Michel, usually you are usually more realistic/conservative on with
> this kind of changes.

Are you saying that I'm less realistic? :)

Marek
Hi Emil,

On 2 March 2018 at 18:38, Emil Velikov <emil.l.velikov@gmail.com> wrote:

> Hi Alex,
>
> On 28 February 2018 at 15:25, Alex Smith <asmith@feralinteractive.com>
> wrote:
> > Hi,
> >
> > Could this (commit 5d61fa4e68b7eb6d481a37efdbb35fdce675a6ad on master)
> be
> > backported to the 17.3 branch to allow it to build with LLVM 6?
> >
> Normally we don't aim to support LLVM versions released after the .0
> Mesa release is out.
> Not that we don't want to - there is simply not enough testing happening.
>
> Sometimes picking the odd build fix is enough, but not always.
>

From my (not particularly extensive) testing, with just this compile fix
radeonsi and radv appear to work OK (radeonsi is functional enough to run
my desktop and radv can run a full game).

It'd be nice to have it able to compile, even if not officially supported.

Thanks,
Alex


>
> As a matter of fact, the only feedback for the AMD drivers status
> (brokenness) is the LunarG testing rig.
>
> Michel, usually you are usually more realistic/conservative on with
> this kind of changes.
> Any objections?
>
> Thanks
> Emil
>
On 2018-03-05 09:51 AM, Alex Smith wrote:
> Hi Emil,
> 
> On 2 March 2018 at 18:38, Emil Velikov <emil.l.velikov@gmail.com
> <mailto:emil.l.velikov@gmail.com>> wrote:
> 
>     Hi Alex,
> 
>     On 28 February 2018 at 15:25, Alex Smith
>     <asmith@feralinteractive.com <mailto:asmith@feralinteractive.com>>
>     wrote:
>     > Hi,
>     >
>     > Could this (commit 5d61fa4e68b7eb6d481a37efdbb35fdce675a6ad on master) be
>     > backported to the 17.3 branch to allow it to build with LLVM 6?
>     >
>     Normally we don't aim to support LLVM versions released after the .0
>     Mesa release is out.
>     Not that we don't want to - there is simply not enough testing
>     happening.
> 
>     Sometimes picking the odd build fix is enough, but not always.
> 
> 
> From my (not particularly extensive) testing, with just this compile fix
> radeonsi and radv appear to work OK (radeonsi is functional enough to
> run my desktop and radv can run a full game).

Mesa 17.3 branched off master on 2017-10-23.

LLVM 6 branched off trunk on 2018-01-03.

In the ~2.5 months between, there could have been any number of
backwards incompatible changes in LLVM trunk, which aren't accounted for
in Mesa 17.3.


> It'd be nice to have it able to compile, even if not officially supported.

I think it's safer to make it clear that Mesa 17.3 doesn't and can't
support LLVM 6.
On 3 March 2018 at 15:40, Marek Olšák <maraeo@gmail.com> wrote:
> On Fri, Mar 2, 2018 at 1:38 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> Hi Alex,
>>
>> On 28 February 2018 at 15:25, Alex Smith <asmith@feralinteractive.com> wrote:
>>> Hi,
>>>
>>> Could this (commit 5d61fa4e68b7eb6d481a37efdbb35fdce675a6ad on master) be
>>> backported to the 17.3 branch to allow it to build with LLVM 6?
>>>
>> Normally we don't aim to support LLVM versions released after the .0
>> Mesa release is out.
>> Not that we don't want to - there is simply not enough testing happening.
>>
>> Sometimes picking the odd build fix is enough, but not always.
>>
>> As a matter of fact, the only feedback for the AMD drivers status
>> (brokenness) is the LunarG testing rig.
>>
>> Michel, usually you are usually more realistic/conservative on with
>> this kind of changes.
>
> Are you saying that I'm less realistic? :)
>
You're right - my wording was bad. I should have only said conservative.

I would love to see agreement within the AMD team - one way or another.
If the decision is to go with these kind of changes, testing will also
be appreciated.

Be that independent individuals, teams, other. Let me ask if the
Lunarg team can add LLVM version to the test matrix.

Thanks
Emil
On 5 March 2018 at 15:13, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 3 March 2018 at 15:40, Marek Olšák <maraeo@gmail.com> wrote:
>> On Fri, Mar 2, 2018 at 1:38 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> Hi Alex,
>>>
>>> On 28 February 2018 at 15:25, Alex Smith <asmith@feralinteractive.com> wrote:
>>>> Hi,
>>>>
>>>> Could this (commit 5d61fa4e68b7eb6d481a37efdbb35fdce675a6ad on master) be
>>>> backported to the 17.3 branch to allow it to build with LLVM 6?
>>>>
>>> Normally we don't aim to support LLVM versions released after the .0
>>> Mesa release is out.
>>> Not that we don't want to - there is simply not enough testing happening.
>>>
>>> Sometimes picking the odd build fix is enough, but not always.
>>>
>>> As a matter of fact, the only feedback for the AMD drivers status
>>> (brokenness) is the LunarG testing rig.
>>>
>>> Michel, usually you are usually more realistic/conservative on with
>>> this kind of changes.
>>
>> Are you saying that I'm less realistic? :)
>>
> You're right - my wording was bad. I should have only said conservative.
>
> I would love to see agreement within the AMD team - one way or another.
> If the decision is to go with these kind of changes, testing will also
> be appreciated.
>
> Be that independent individuals, teams, other. Let me ask if the
> Lunarg team can add LLVM version to the test matrix.
>
Have some good news - the Lunarg team will add LLVM 6.0 in the list.
So as soon as we get that + there's no glaring regressions I think
we'll be in decent shape.

For anyone wondering why I tend towards the conservative side:
 - making it build, hence having partial LLVM X support is not enough
The 'partial' word will be missed and you'll get plenty of unhappy
users as regressions happen

 - missing a wide/popular test base
The odd report of game X working fine is _greatly_ appreciated, yet
quite limited

 - test results are not [easily] accessible by many people
We want something to refer to as we decide to allow (or forbid) LLVM X

HTH
Emil
I can test piglit+CTS+deqp on the GPU that I have. (currently Polaris12)

Marek

On Mar 7, 2018 10:53 AM, "Emil Velikov" <emil.l.velikov@gmail.com> wrote:

> On 5 March 2018 at 15:13, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > On 3 March 2018 at 15:40, Marek Olšák <maraeo@gmail.com> wrote:
> >> On Fri, Mar 2, 2018 at 1:38 PM, Emil Velikov <emil.l.velikov@gmail.com>
> wrote:
> >>> Hi Alex,
> >>>
> >>> On 28 February 2018 at 15:25, Alex Smith <asmith@feralinteractive.com>
> wrote:
> >>>> Hi,
> >>>>
> >>>> Could this (commit 5d61fa4e68b7eb6d481a37efdbb35fdce675a6ad on
> master) be
> >>>> backported to the 17.3 branch to allow it to build with LLVM 6?
> >>>>
> >>> Normally we don't aim to support LLVM versions released after the .0
> >>> Mesa release is out.
> >>> Not that we don't want to - there is simply not enough testing
> happening.
> >>>
> >>> Sometimes picking the odd build fix is enough, but not always.
> >>>
> >>> As a matter of fact, the only feedback for the AMD drivers status
> >>> (brokenness) is the LunarG testing rig.
> >>>
> >>> Michel, usually you are usually more realistic/conservative on with
> >>> this kind of changes.
> >>
> >> Are you saying that I'm less realistic? :)
> >>
> > You're right - my wording was bad. I should have only said conservative.
> >
> > I would love to see agreement within the AMD team - one way or another.
> > If the decision is to go with these kind of changes, testing will also
> > be appreciated.
> >
> > Be that independent individuals, teams, other. Let me ask if the
> > Lunarg team can add LLVM version to the test matrix.
> >
> Have some good news - the Lunarg team will add LLVM 6.0 in the list.
> So as soon as we get that + there's no glaring regressions I think
> we'll be in decent shape.
>
> For anyone wondering why I tend towards the conservative side:
>  - making it build, hence having partial LLVM X support is not enough
> The 'partial' word will be missed and you'll get plenty of unhappy
> users as regressions happen
>
>  - missing a wide/popular test base
> The odd report of game X working fine is _greatly_ appreciated, yet
> quite limited
>
>  - test results are not [easily] accessible by many people
> We want something to refer to as we decide to allow (or forbid) LLVM X
>
> HTH
> Emil
>
[Adding Andres, Juan since they'll be doing the next 17.3]

On 7 March 2018 at 16:35, Marek Olšák <maraeo@gmail.com> wrote:
> I can test piglit+CTS+deqp on the GPU that I have. (currently Polaris12)
>
Yes please - that sounds great. If we have more than one device that
would be amazing.
FTR the Lunarg team seems to be ready on their side.

So with the a) compliance testing from Marek, b) games coverage from
Lunarg and c) in-house testing from Alex, we'll be solid.

I'm pretty sure Michel and others will agree ;-)

Thanks
Emil
Piglit+CTS+dEQP look good on Polaris12 with Mesa 17.3 and LLVM 6.0.

Marek

On Thu, Mar 8, 2018 at 6:22 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> [Adding Andres, Juan since they'll be doing the next 17.3]
>
> On 7 March 2018 at 16:35, Marek Olšák <maraeo@gmail.com> wrote:
>> I can test piglit+CTS+deqp on the GPU that I have. (currently Polaris12)
>>
> Yes please - that sounds great. If we have more than one device that
> would be amazing.
> FTR the Lunarg team seems to be ready on their side.
>
> So with the a) compliance testing from Marek, b) games coverage from
> Lunarg and c) in-house testing from Alex, we'll be solid.
>
> I'm pretty sure Michel and others will agree ;-)
>
> Thanks
> Emil
On 13 March 2018 at 15:40, Marek Olšák <maraeo@gmail.com> wrote:
> Piglit+CTS+dEQP look good on Polaris12 with Mesa 17.3 and LLVM 6.0.
>
Thank you very much Marek!

Meanwhile I've gone through the results in the LunarG tool. Everything
looks good for the 88 games tested.
Feel free to login and check if you're interested.

I believe Juan is chasing regressions elsewhere in the tree, in
preparation for 117.3.7.
Hopefully they will be sorted quickly.

-Emil
On Tue, 2018-03-13 at 16:53 +0000, Emil Velikov wrote:
> On 13 March 2018 at 15:40, Marek Olšák <maraeo@gmail.com> wrote:
> > Piglit+CTS+dEQP look good on Polaris12 with Mesa 17.3 and LLVM 6.0.
> > 
> 
> Thank you very much Marek!
> 
> Meanwhile I've gone through the results in the LunarG tool. Everything
> looks good for the 88 games tested.
> Feel free to login and check if you're interested.
> 

That's nice! Thanks both.


> I believe Juan is chasing regressions elsewhere in the tree, in
> preparation for 117.3.7.
> 

17.3.7, actually :D


Yes, after dealing with some regressions, I think now everything is fine.
Waiting for Intel CI to confirm it is OK, and make the pre-release.

	J.A.

> Hopefully they will be sorted quickly.
> 
> -Emil
>