llvm dev - Feb 2018 - [RFC] Should we bump the bitcode version in LLVM 6.0?

Hi,

TL;DR
r317488 changed the way fast math flags are laid out in the bitcode and anyone
compiling a pre-llvm-6.0 bitcode with llvm-6.0 will lose all the optimizations
guarded by isFast and a pre-llvm-6.0 compiler compiling a llvm-6.0 bitcode will
potentially generate incorrect code w.r.t. fast math expectations.

Should we bump the bitcode version because of that and have the autoupgrader
properly rewrite the fast-math to preserve that semantic?
(I believe we should!)


* Context *

With https://reviews.llvm.org/D39304 <https://reviews.llvm.org/D39304> /
r317488 we got rid of the umbrella UnsafeMath flag and introduced 3 more flags
that better represent the different things that happen under fast-math.

From a bitcode perspective, this change looks like this:
Before r317488 we had 6 bits that respectively represented:

UnsafeMath
nnan
ninf
nsz
arcp
contract
*unset*

(The order may not match what is exactly in the bitcode.)

After r317488 we had 7 bits that respectively represented:
reassoc (-UnsafeMath- is gone)
nnan
ninf
nsz
arcp
contract
*afn* (new bit)

Before r317488, fast-math was true if UnsafeMath was true (this should also
imply all the other flags are sets). After r317488, fast-math is true if all the
bits are set, in particular the afn, new one, too.


* Problem *

Given we currently have no way to check if a bitcode file has been generated
pre-r317488 or post-r317488 that means that:
1. a post-r317488 compiler is going to skip any optimization guarded by isFast
for all pre-r317488 bitcode file (remember the afn bit is not set here)
2. a pre-r317488 compiler is going to run any optimization guarded by
unsafeAlgebra for any post-r317488 bitcode file that has the reassoc bit
(remember we repurposed UnsafeMath)

Scenario #2 might be unlikely but we’re potentially breaking the semantic of the
program. It is particularly dangerous because there is nothing that is going to
tell us that we are in this situation “downgrade" situation.
#1 means that any code that uses unsafeMath is going to get a performance hit.

In other words, one scenario implies generating wrong code and the other,
runtime performance regressions.


* Feedback Needed *

I believe this change is big enough that it would be worth bumping the bitcode
version so that the upgrader can do the right thing *before* we release it to
the public with LLVM-6.0.

That being said, I don’t know what are the implications of such bump and if
people really don’t care about the performance problem that might be okay. The
silent downgrade path is however concerning.

Should we bump the bitcode version because of that change and have the
autoupgrader properly rewrite the fast-math flags to
preserve the semantic and make sure there are no silent downgrade?


Thanks,
-Quentin
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180208/88412a27/attachment.html>

Hi Quentin,
> r317488 changed the way fast math flags are laid out in the bitcode and
anyone
> compiling a pre-llvm-6.0 bitcode with llvm-6.0 will lose all the
optimizations guarded
> by isFast and a pre-llvm-6.0 compiler compiling a llvm-6.0 bitcode will
potentially
> generate incorrect code w.r.t. fast math expectations.
>
> Should we bump the bitcode version because of that and have the
autoupgrader properly
> rewrite the fast-math to preserve that semantic?
> (I believe we should!)
I agree.

Since I was involved in the discussions that motivated the change of r317488, I
feel compelled to chime in.

The danger you described of a pre-llvm-6.0 compiler compiling llvm-6.0 bitcode
with the reassoc bit enabled (but not the entire set of fast-math-flags
enabled), incorrectly behaving as though all of fast-math is enabled, is pretty
compelling.

As an aside, at my employer (Sony), this isn't a critical issue for our
product.  But that's just because out of an abundance of caution, we only
permit LTO using bitcode files built using the same llvm version, irrespective
of the bitcode version of the IR.  That is, when doing LTO, even if the bitcode
version isn't bumped, our llvm-5.0 based compiler will reject llvm-6.0
bitcode, and our llvm-6.0 based compiler will reject llvm-5.0 bitcode.  So if
there is a strong feeling by others that the possibility of users compiling new
bitcode with an older compiler is insignificant, then I won't argue that we
must bump the IR version markers.

But given your points, it seems to me bumping the bitcode version is the “right
thing to do”.  And from the discussion at https://reviews.llvm.org/D39304,
apparently Michael already has the patch ready, so this can be addressed
quickly.

Thanks,
-Warren


From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of Quentin
Colombet via llvm-dev
Sent: Thursday, February 8, 2018 5:34 PM
To: llvm-dev
Cc: Michael Berg
Subject: [llvm-dev] [RFC] Should we bump the bitcode version in LLVM 6.0?

Hi,

TL;DR
r317488 changed the way fast math flags are laid out in the bitcode and anyone
compiling a pre-llvm-6.0 bitcode with llvm-6.0 will lose all the optimizations
guarded by isFast and a pre-llvm-6.0 compiler compiling a llvm-6.0 bitcode will
potentially generate incorrect code w.r.t. fast math expectations.

Should we bump the bitcode version because of that and have the autoupgrader
properly rewrite the fast-math to preserve that semantic?
(I believe we should!)


* Context *

With https://reviews.llvm.org/D39304 / r317488 we got rid of the umbrella
UnsafeMath flag and introduced 3 more flags that better represent the different
things that happen under fast-math.

From a bitcode perspective, this change looks like this:
Before r317488 we had 6 bits that respectively represented:

UnsafeMath
nnan
ninf
nsz
arcp
contract
*unset*

(The order may not match what is exactly in the bitcode.)

After r317488 we had 7 bits that respectively represented:
reassoc (-UnsafeMath- is gone)
nnan
ninf
nsz
arcp
contract
*afn* (new bit)

Before r317488, fast-math was true if UnsafeMath was true (this should also
imply all the other flags are sets). After r317488, fast-math is true if all the
bits are set, in particular the afn, new one, too.


* Problem *

Given we currently have no way to check if a bitcode file has been generated
pre-r317488 or post-r317488 that means that:
1. a post-r317488 compiler is going to skip any optimization guarded by isFast
for all pre-r317488 bitcode file (remember the afn bit is not set here)
2. a pre-r317488 compiler is going to run any optimization guarded by
unsafeAlgebra for any post-r317488 bitcode file that has the reassoc bit
(remember we repurposed UnsafeMath)

Scenario #2 might be unlikely but we’re potentially breaking the semantic of the
program. It is particularly dangerous because there is nothing that is going to
tell us that we are in this situation “downgrade" situation.
#1 means that any code that uses unsafeMath is going to get a performance hit.

In other words, one scenario implies generating wrong code and the other,
runtime performance regressions.


* Feedback Needed *

I believe this change is big enough that it would be worth bumping the bitcode
version so that the upgrader can do the right thing *before* we release it to
the public with LLVM-6.0.

That being said, I don’t know what are the implications of such bump and if
people really don’t care about the performance problem that might be okay. The
silent downgrade path is however concerning.

Should we bump the bitcode version because of that change and have the
autoupgrader properly rewrite the fast-math flags to
preserve the semantic and make sure there are no silent downgrade?


Thanks,
-Quentin
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180209/536ffc07/attachment.html>

Just wanted to point out part of this even becoming a problem is the use of
`isFast()`.
There should be warnings against using isFast() and the existing code should be
changed to query specific flags instead...

- Matthias
> On Feb 8, 2018, at 5:34 PM, Quentin Colombet via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi,
> 
> TL;DR
> r317488 changed the way fast math flags are laid out in the bitcode and
anyone compiling a pre-llvm-6.0 bitcode with llvm-6.0 will lose all the
optimizations guarded by isFast and a pre-llvm-6.0 compiler compiling a llvm-6.0
bitcode will potentially generate incorrect code w.r.t. fast math expectations.
> 
> Should we bump the bitcode version because of that and have the
autoupgrader properly rewrite the fast-math to preserve that semantic?
> (I believe we should!)
> 
> 
> * Context *
> 
> With https://reviews.llvm.org/D39304
<https://reviews.llvm.org/D39304> / r317488 we got rid of the umbrella
UnsafeMath flag and introduced 3 more flags that better represent the different
things that happen under fast-math.
> 
> From a bitcode perspective, this change looks like this:
> Before r317488 we had 6 bits that respectively represented:
> 
> UnsafeMath
> nnan
> ninf
> nsz
> arcp
> contract
> *unset*
> 
> (The order may not match what is exactly in the bitcode.)
> 
> After r317488 we had 7 bits that respectively represented:
> reassoc (-UnsafeMath- is gone)
> nnan
> ninf
> nsz
> arcp
> contract
> *afn* (new bit)
> 
> Before r317488, fast-math was true if UnsafeMath was true (this should also
imply all the other flags are sets). After r317488, fast-math is true if all the
bits are set, in particular the afn, new one, too.
> 
> 
> * Problem *
> 
> Given we currently have no way to check if a bitcode file has been
generated pre-r317488 or post-r317488 that means that:
> 1. a post-r317488 compiler is going to skip any optimization guarded by
isFast for all pre-r317488 bitcode file (remember the afn bit is not set here)
> 2. a pre-r317488 compiler is going to run any optimization guarded by
unsafeAlgebra for any post-r317488 bitcode file that has the reassoc bit
(remember we repurposed UnsafeMath)
> 
> Scenario #2 might be unlikely but we’re potentially breaking the semantic
of the program. It is particularly dangerous because there is nothing that is
going to tell us that we are in this situation “downgrade" situation.
> #1 means that any code that uses unsafeMath is going to get a performance
hit.
> 
> In other words, one scenario implies generating wrong code and the other,
runtime performance regressions.
> 
> 
> * Feedback Needed *
> 
> I believe this change is big enough that it would be worth bumping the
bitcode version so that the upgrader can do the right thing *before* we release
it to the public with LLVM-6.0.
> 
> That being said, I don’t know what are the implications of such bump and if
people really don’t care about the performance problem that might be okay. The
silent downgrade path is however concerning.
> 
> Should we bump the bitcode version because of that change and have the
autoupgrader properly rewrite the fast-math flags to
> preserve the semantic and make sure there are no silent downgrade?
> 
> 
> Thanks,
> -Quentin
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180209/2113d2ac/attachment-0001.html>

Does the language reference need to be updated?

http://llvm.org/docs/LangRef.html#fast-math-flags

It still mentions "fast"



On Thu, Feb 8, 2018 at 8:34 PM, Quentin Colombet via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi,
>
> TL;DR
> r317488 changed the way fast math flags are laid out in the bitcode and
> anyone compiling a pre-llvm-6.0 bitcode with llvm-6.0 will lose all the
> optimizations guarded by isFast and a pre-llvm-6.0 compiler compiling a
> llvm-6.0 bitcode will potentially generate incorrect code w.r.t. fast math
> expectations.
>
> Should we bump the bitcode version because of that and have the
> autoupgrader properly rewrite the fast-math to preserve that semantic?
> (I believe we should!)
>
>
> * Context *
>
> With https://reviews.llvm.org/D39304 / r317488 we got rid of the umbrella
> UnsafeMath flag and introduced 3 more flags that better represent the
> different things that happen under fast-math.
>
> From a bitcode perspective, this change looks like this:
> Before r317488 we had 6 bits that respectively represented:
>
> UnsafeMath
> nnan
> ninf
> nsz
> arcp
> contract
> *unset*
>
> (The order may not match what is exactly in the bitcode.)
>
> After r317488 we had 7 bits that respectively represented:
> reassoc (-UnsafeMath- is gone)
> nnan
> ninf
> nsz
> arcp
> contract
> *afn* (new bit)
>
> Before r317488, fast-math was true if UnsafeMath was true (this should
> also imply all the other flags are sets). After r317488, fast-math is true
> if all the bits are set, in particular the afn, new one, too.
>
>
> * Problem *
>
> Given we currently have no way to check if a bitcode file has been
> generated pre-r317488 or post-r317488 that means that:
> 1. a post-r317488 compiler is going to skip any optimization guarded by
> isFast for all pre-r317488 bitcode file (remember the afn bit is not set
> here)
> 2. a pre-r317488 compiler is going to run any optimization guarded by
> unsafeAlgebra for any post-r317488 bitcode file that has the reassoc bit
> (remember we repurposed UnsafeMath)
>
> Scenario #2 might be unlikely but we’re potentially breaking the semantic
> of the program. It is particularly dangerous because there is nothing that
> is going to tell us that we are in this situation “downgrade"
situation.
> #1 means that any code that uses unsafeMath is going to get a performance
> hit.
>
> In other words, one scenario implies generating wrong code and the other,
> runtime performance regressions.
>
>
> * Feedback Needed *
>
> I believe this change is big enough that it would be worth bumping the
> bitcode version so that the upgrader can do the right thing *before* we
> release it to the public with LLVM-6.0.
>
> That being said, I don’t know what are the implications of such bump and
> if people really don’t care about the performance problem that might be
> okay. The silent downgrade path is however concerning.
>
> Should we bump the bitcode version because of that change and have the
> autoupgrader properly rewrite the fast-math flags to
> preserve the semantic and make sure there are no silent downgrade?
>
>
> Thanks,
> -Quentin
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180209/0b881b04/attachment.html>

I agree about isFast(), but determining the minimal subset of flags needed
to replace those calls requires some thought.

I did start looking at replacing isFast() in the reassociation pass. In
theory, we should be able to do those transforms with just 'reassoc'
now,
but I hit complications with intersecting FMF of different instructions.

Conservatively, we could just ask if everything but the new 'afn' is set
for folds that do not include mathlib or intrinsic math functions. That
could be an alternative to versioning the IR...unless the perf problems are
actually caused by not folding mathlib calls now?

As I said in D39304, I have no objection to versioning the IR, but I also
don't know if there's a cost/downside to that.


On Fri, Feb 9, 2018 at 11:05 AM, Matthias Braun via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Just wanted to point out part of this even becoming a problem is the use
> of `isFast()`.
> There should be warnings against using isFast() and the existing code
> should be changed to query specific flags instead...
>
> - Matthias
>
> On Feb 8, 2018, at 5:34 PM, Quentin Colombet via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> Hi,
>
> TL;DR
> r317488 changed the way fast math flags are laid out in the bitcode and
> anyone compiling a pre-llvm-6.0 bitcode with llvm-6.0 will lose all the
> optimizations guarded by isFast and a pre-llvm-6.0 compiler compiling a
> llvm-6.0 bitcode will potentially generate incorrect code w.r.t. fast math
> expectations.
>
> Should we bump the bitcode version because of that and have the
> autoupgrader properly rewrite the fast-math to preserve that semantic?
> (I believe we should!)
>
>
> * Context *
>
> With https://reviews.llvm.org/D39304 / r317488 we got rid of the umbrella
> UnsafeMath flag and introduced 3 more flags that better represent the
> different things that happen under fast-math.
>
> From a bitcode perspective, this change looks like this:
> Before r317488 we had 6 bits that respectively represented:
>
> UnsafeMath
> nnan
> ninf
> nsz
> arcp
> contract
> *unset*
>
> (The order may not match what is exactly in the bitcode.)
>
> After r317488 we had 7 bits that respectively represented:
> reassoc (-UnsafeMath- is gone)
> nnan
> ninf
> nsz
> arcp
> contract
> *afn* (new bit)
>
> Before r317488, fast-math was true if UnsafeMath was true (this should
> also imply all the other flags are sets). After r317488, fast-math is true
> if all the bits are set, in particular the afn, new one, too.
>
>
> * Problem *
>
> Given we currently have no way to check if a bitcode file has been
> generated pre-r317488 or post-r317488 that means that:
> 1. a post-r317488 compiler is going to skip any optimization guarded by
> isFast for all pre-r317488 bitcode file (remember the afn bit is not set
> here)
> 2. a pre-r317488 compiler is going to run any optimization guarded by
> unsafeAlgebra for any post-r317488 bitcode file that has the reassoc bit
> (remember we repurposed UnsafeMath)
>
> Scenario #2 might be unlikely but we’re potentially breaking the semantic
> of the program. It is particularly dangerous because there is nothing that
> is going to tell us that we are in this situation “downgrade"
situation.
> #1 means that any code that uses unsafeMath is going to get a performance
> hit.
>
> In other words, one scenario implies generating wrong code and the other,
> runtime performance regressions.
>
>
> * Feedback Needed *
>
> I believe this change is big enough that it would be worth bumping the
> bitcode version so that the upgrader can do the right thing *before* we
> release it to the public with LLVM-6.0.
>
> That being said, I don’t know what are the implications of such bump and
> if people really don’t care about the performance problem that might be
> okay. The silent downgrade path is however concerning.
>
> Should we bump the bitcode version because of that change and have the
> autoupgrader properly rewrite the fast-math flags to
> preserve the semantic and make sure there are no silent downgrade?
>
>
> Thanks,
> -Quentin
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180209/39a7b7f0/attachment.html>

No, I think that's ok as-is. We still allow 'fast' in IR text as a
shortcut
meaning all of the other bits are set. It's just encoded differently in
binary form.

On Fri, Feb 9, 2018 at 11:07 AM, Andrew Kelley via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Does the language reference need to be updated?
>
> http://llvm.org/docs/LangRef.html#fast-math-flags
>
> It still mentions "fast"
>
>
>
> On Thu, Feb 8, 2018 at 8:34 PM, Quentin Colombet via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Hi,
>>
>> TL;DR
>> r317488 changed the way fast math flags are laid out in the bitcode and
>> anyone compiling a pre-llvm-6.0 bitcode with llvm-6.0 will lose all the
>> optimizations guarded by isFast and a pre-llvm-6.0 compiler compiling a
>> llvm-6.0 bitcode will potentially generate incorrect code w.r.t. fast
math
>> expectations.
>>
>> Should we bump the bitcode version because of that and have the
>> autoupgrader properly rewrite the fast-math to preserve that semantic?
>> (I believe we should!)
>>
>>
>> * Context *
>>
>> With https://reviews.llvm.org/D39304 / r317488 we got rid of the
>> umbrella UnsafeMath flag and introduced 3 more flags that better
represent
>> the different things that happen under fast-math.
>>
>> From a bitcode perspective, this change looks like this:
>> Before r317488 we had 6 bits that respectively represented:
>>
>> UnsafeMath
>> nnan
>> ninf
>> nsz
>> arcp
>> contract
>> *unset*
>>
>> (The order may not match what is exactly in the bitcode.)
>>
>> After r317488 we had 7 bits that respectively represented:
>> reassoc (-UnsafeMath- is gone)
>> nnan
>> ninf
>> nsz
>> arcp
>> contract
>> *afn* (new bit)
>>
>> Before r317488, fast-math was true if UnsafeMath was true (this should
>> also imply all the other flags are sets). After r317488, fast-math is
true
>> if all the bits are set, in particular the afn, new one, too.
>>
>>
>> * Problem *
>>
>> Given we currently have no way to check if a bitcode file has been
>> generated pre-r317488 or post-r317488 that means that:
>> 1. a post-r317488 compiler is going to skip any optimization guarded by
>> isFast for all pre-r317488 bitcode file (remember the afn bit is not
set
>> here)
>> 2. a pre-r317488 compiler is going to run any optimization guarded by
>> unsafeAlgebra for any post-r317488 bitcode file that has the reassoc
bit
>> (remember we repurposed UnsafeMath)
>>
>> Scenario #2 might be unlikely but we’re potentially breaking the
semantic
>> of the program. It is particularly dangerous because there is nothing
that
>> is going to tell us that we are in this situation “downgrade"
situation.
>> #1 means that any code that uses unsafeMath is going to get a
performance
>> hit.
>>
>> In other words, one scenario implies generating wrong code and the
other,
>> runtime performance regressions.
>>
>>
>> * Feedback Needed *
>>
>> I believe this change is big enough that it would be worth bumping the
>> bitcode version so that the upgrader can do the right thing *before* we
>> release it to the public with LLVM-6.0.
>>
>> That being said, I don’t know what are the implications of such bump
and
>> if people really don’t care about the performance problem that might be
>> okay. The silent downgrade path is however concerning.
>>
>> Should we bump the bitcode version because of that change and have the
>> autoupgrader properly rewrite the fast-math flags to
>> preserve the semantic and make sure there are no silent downgrade?
>>
>>
>> Thanks,
>> -Quentin
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180209/3fee1771/attachment.html>

Agree, but that wouldn’t solve the downgrade problem plus this is probably too
late to fix LLVM 6.0 for all the isFast uses.
> On Feb 9, 2018, at 10:05 AM, Matthias Braun <mbraun at apple.com>
wrote:
> 
> Just wanted to point out part of this even becoming a problem is the use of
`isFast()`.
> There should be warnings against using isFast() and the existing code
should be changed to query specific flags instead...
> 
> - Matthias
> 
>> On Feb 8, 2018, at 5:34 PM, Quentin Colombet via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>> 
>> Hi,
>> 
>> TL;DR
>> r317488 changed the way fast math flags are laid out in the bitcode and
anyone compiling a pre-llvm-6.0 bitcode with llvm-6.0 will lose all the
optimizations guarded by isFast and a pre-llvm-6.0 compiler compiling a llvm-6.0
bitcode will potentially generate incorrect code w.r.t. fast math expectations.
>> 
>> Should we bump the bitcode version because of that and have the
autoupgrader properly rewrite the fast-math to preserve that semantic?
>> (I believe we should!)
>> 
>> 
>> * Context *
>> 
>> With https://reviews.llvm.org/D39304
<https://reviews.llvm.org/D39304> / r317488 we got rid of the umbrella
UnsafeMath flag and introduced 3 more flags that better represent the different
things that happen under fast-math.
>> 
>> From a bitcode perspective, this change looks like this:
>> Before r317488 we had 6 bits that respectively represented:
>> 
>> UnsafeMath
>> nnan
>> ninf
>> nsz
>> arcp
>> contract
>> *unset*
>> 
>> (The order may not match what is exactly in the bitcode.)
>> 
>> After r317488 we had 7 bits that respectively represented:
>> reassoc (-UnsafeMath- is gone)
>> nnan
>> ninf
>> nsz
>> arcp
>> contract
>> *afn* (new bit)
>> 
>> Before r317488, fast-math was true if UnsafeMath was true (this should
also imply all the other flags are sets). After r317488, fast-math is true if
all the bits are set, in particular the afn, new one, too.
>> 
>> 
>> * Problem *
>> 
>> Given we currently have no way to check if a bitcode file has been
generated pre-r317488 or post-r317488 that means that:
>> 1. a post-r317488 compiler is going to skip any optimization guarded by
isFast for all pre-r317488 bitcode file (remember the afn bit is not set here)
>> 2. a pre-r317488 compiler is going to run any optimization guarded by
unsafeAlgebra for any post-r317488 bitcode file that has the reassoc bit
(remember we repurposed UnsafeMath)
>> 
>> Scenario #2 might be unlikely but we’re potentially breaking the
semantic of the program. It is particularly dangerous because there is nothing
that is going to tell us that we are in this situation “downgrade"
situation.
>> #1 means that any code that uses unsafeMath is going to get a
performance hit.
>> 
>> In other words, one scenario implies generating wrong code and the
other, runtime performance regressions.
>> 
>> 
>> * Feedback Needed *
>> 
>> I believe this change is big enough that it would be worth bumping the
bitcode version so that the upgrader can do the right thing *before* we release
it to the public with LLVM-6.0.
>> 
>> That being said, I don’t know what are the implications of such bump
and if people really don’t care about the performance problem that might be
okay. The silent downgrade path is however concerning.
>> 
>> Should we bump the bitcode version because of that change and have the
autoupgrader properly rewrite the fast-math flags to
>> preserve the semantic and make sure there are no silent downgrade?
>> 
>> 
>> Thanks,
>> -Quentin
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180209/d2acebdd/attachment.html>

2018-02-08 17:34 GMT-08:00 Quentin Colombet via llvm-dev <
llvm-dev at lists.llvm.org>:
> Hi,
>
> TL;DR
> r317488 changed the way fast math flags are laid out in the bitcode and
> anyone compiling a pre-llvm-6.0 bitcode with llvm-6.0 will lose all the
> optimizations guarded by isFast and a pre-llvm-6.0 compiler compiling a
> llvm-6.0 bitcode will potentially generate incorrect code w.r.t. fast math
> expectations.
>
> Should we bump the bitcode version because of that and have the
> autoupgrader properly rewrite the fast-math to preserve that semantic?
> (I believe we should!)
>
>
> * Context *
>
> With https://reviews.llvm.org/D39304 / r317488 we got rid of the umbrella
> UnsafeMath flag and introduced 3 more flags that better represent the
> different things that happen under fast-math.
>
> From a bitcode perspective, this change looks like this:
> Before r317488 we had 6 bits that respectively represented:
>
> UnsafeMath
> nnan
> ninf
> nsz
> arcp
> contract
> *unset*
>
> (The order may not match what is exactly in the bitcode.)
>
> After r317488 we had 7 bits that respectively represented:
> reassoc (-UnsafeMath- is gone)
> nnan
> ninf
> nsz
> arcp
> contract
> *afn* (new bit)
>
> Before r317488, fast-math was true if UnsafeMath was true (this should
> also imply all the other flags are sets). After r317488, fast-math is true
> if all the bits are set, in particular the afn, new one, too.
>
>
> * Problem *
>
> Given we currently have no way to check if a bitcode file has been
> generated pre-r317488 or post-r317488 that means that:
> 1. a post-r317488 compiler is going to skip any optimization guarded by
> isFast for all pre-r317488 bitcode file (remember the afn bit is not set
> here)
> 2. a pre-r317488 compiler is going to run any optimization guarded by
> unsafeAlgebra for any post-r317488 bitcode file that has the reassoc bit
> (remember we repurposed UnsafeMath)
>
> Scenario #2 might be unlikely but we’re potentially breaking the semantic
> of the program. It is particularly dangerous because there is nothing that
> is going to tell us that we are in this situation “downgrade"
situation.
> #1 means that any code that uses unsafeMath is going to get a performance
> hit.
>
> In other words, one scenario implies generating wrong code and the other,
> runtime performance regressions.
>


Scenario #1 is unsupported AFAIK, unless I missed something the bitcode is
not forward compatible: loading newer bitcode with an older LLVM has never
been supported as far I can remember.

Scenario #2 is very much like other performance regression when we drop old
metadata (i.e. bitcode upgrade isn't performance proof in general but only
"best effort", there have been multiple instance of this in the past).

Usually IIRC we try not to version the bitcode at all this way (i.e.
bitcode does not have a linear versioning that is regularly bumped) but
instead make sure the encoding itself allows an easy upgrade. I.e. the
encoding of the FMF should have been such that the reader can detect and
upgrade to the new IR representation.
Now this is too late here I guess, so bumping may be a possible trade-off.
What about any bitcode shipped after r317488 but before the version bump?
Not worth taking into account because of the short period of time?

Best,

-- 
Mehdi

>
>
> * Feedback Needed *
>
> I believe this change is big enough that it would be worth bumping the
> bitcode version so that the upgrader can do the right thing *before* we
> release it to the public with LLVM-6.0.
>
> That being said, I don’t know what are the implications of such bump and
> if people really don’t care about the performance problem that might be
> okay. The silent downgrade path is however concerning.
>
> Should we bump the bitcode version because of that change and have the
> autoupgrader properly rewrite the fast-math flags to
> preserve the semantic and make sure there are no silent downgrade?
>
>
> Thanks,
> -Quentin
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180213/4a1263ce/attachment.html>

For all who want to participate in the discussion there is a review open now
regarding this topic:

https://reviews.llvm.org/D43253 <https://reviews.llvm.org/D43253>

Regards,
Michael
> On Feb 13, 2018, at 12:34 PM, Mehdi AMINI <joker.eph at gmail.com>
wrote:
> 
> 
> 
> 2018-02-08 17:34 GMT-08:00 Quentin Colombet via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>:
> Hi,
> 
> TL;DR
> r317488 changed the way fast math flags are laid out in the bitcode and
anyone compiling a pre-llvm-6.0 bitcode with llvm-6.0 will lose all the
optimizations guarded by isFast and a pre-llvm-6.0 compiler compiling a llvm-6.0
bitcode will potentially generate incorrect code w.r.t. fast math expectations.
> 
> Should we bump the bitcode version because of that and have the
autoupgrader properly rewrite the fast-math to preserve that semantic?
> (I believe we should!)
> 
> 
> * Context *
> 
> With https://reviews.llvm.org/D39304
<https://reviews.llvm.org/D39304> / r317488 we got rid of the umbrella
UnsafeMath flag and introduced 3 more flags that better represent the different
things that happen under fast-math.
> 
> From a bitcode perspective, this change looks like this:
> Before r317488 we had 6 bits that respectively represented:
> 
> UnsafeMath
> nnan
> ninf
> nsz
> arcp
> contract
> *unset*
> 
> (The order may not match what is exactly in the bitcode.)
> 
> After r317488 we had 7 bits that respectively represented:
> reassoc (-UnsafeMath- is gone)
> nnan
> ninf
> nsz
> arcp
> contract
> *afn* (new bit)
> 
> Before r317488, fast-math was true if UnsafeMath was true (this should also
imply all the other flags are sets). After r317488, fast-math is true if all the
bits are set, in particular the afn, new one, too.
> 
> 
> * Problem *
> 
> Given we currently have no way to check if a bitcode file has been
generated pre-r317488 or post-r317488 that means that:
> 1. a post-r317488 compiler is going to skip any optimization guarded by
isFast for all pre-r317488 bitcode file (remember the afn bit is not set here)
> 2. a pre-r317488 compiler is going to run any optimization guarded by
unsafeAlgebra for any post-r317488 bitcode file that has the reassoc bit
(remember we repurposed UnsafeMath)
> 
> Scenario #2 might be unlikely but we’re potentially breaking the semantic
of the program. It is particularly dangerous because there is nothing that is
going to tell us that we are in this situation “downgrade" situation.
> #1 means that any code that uses unsafeMath is going to get a performance
hit.
> 
> In other words, one scenario implies generating wrong code and the other,
runtime performance regressions.
> 
> 
> 
> Scenario #1 is unsupported AFAIK, unless I missed something the bitcode is
not forward compatible: loading newer bitcode with an older LLVM has never been
supported as far I can remember.
> 
> Scenario #2 is very much like other performance regression when we drop old
metadata (i.e. bitcode upgrade isn't performance proof in general but only
"best effort", there have been multiple instance of this in the past).
> 
> Usually IIRC we try not to version the bitcode at all this way (i.e.
bitcode does not have a linear versioning that is regularly bumped) but instead
make sure the encoding itself allows an easy upgrade. I.e. the encoding of the
FMF should have been such that the reader can detect and upgrade to the new IR
representation.
> Now this is too late here I guess, so bumping may be a possible trade-off.
What about any bitcode shipped after r317488 but before the version bump? Not
worth taking into account because of the short period of time?
> 
> Best,
> 
> -- 
> Mehdi
>  
> 
> 
> * Feedback Needed *
> 
> I believe this change is big enough that it would be worth bumping the
bitcode version so that the upgrader can do the right thing *before* we release
it to the public with LLVM-6.0.
> 
> That being said, I don’t know what are the implications of such bump and if
people really don’t care about the performance problem that might be okay. The
silent downgrade path is however concerning.
> 
> Should we bump the bitcode version because of that change and have the
autoupgrader properly rewrite the fast-math flags to
> preserve the semantic and make sure there are no silent downgrade?
> 
> 
> Thanks,
> -Quentin
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180213/96234a7e/attachment.html>

Hi Mehdi,

> On Feb 13, 2018, at 12:34 PM, Mehdi AMINI <joker.eph at gmail.com>
wrote:
> 
> 
> 
> 2018-02-08 17:34 GMT-08:00 Quentin Colombet via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>:
> Hi,
> 
> TL;DR
> r317488 changed the way fast math flags are laid out in the bitcode and
anyone compiling a pre-llvm-6.0 bitcode with llvm-6.0 will lose all the
optimizations guarded by isFast and a pre-llvm-6.0 compiler compiling a llvm-6.0
bitcode will potentially generate incorrect code w.r.t. fast math expectations.
> 
> Should we bump the bitcode version because of that and have the
autoupgrader properly rewrite the fast-math to preserve that semantic?
> (I believe we should!)
> 
> 
> * Context *
> 
> With https://reviews.llvm.org/D39304
<https://reviews.llvm.org/D39304> / r317488 we got rid of the umbrella
UnsafeMath flag and introduced 3 more flags that better represent the different
things that happen under fast-math.
> 
> From a bitcode perspective, this change looks like this:
> Before r317488 we had 6 bits that respectively represented:
> 
> UnsafeMath
> nnan
> ninf
> nsz
> arcp
> contract
> *unset*
> 
> (The order may not match what is exactly in the bitcode.)
> 
> After r317488 we had 7 bits that respectively represented:
> reassoc (-UnsafeMath- is gone)
> nnan
> ninf
> nsz
> arcp
> contract
> *afn* (new bit)
> 
> Before r317488, fast-math was true if UnsafeMath was true (this should also
imply all the other flags are sets). After r317488, fast-math is true if all the
bits are set, in particular the afn, new one, too.
> 
> 
> * Problem *
> 
> Given we currently have no way to check if a bitcode file has been
generated pre-r317488 or post-r317488 that means that:
> 1. a post-r317488 compiler is going to skip any optimization guarded by
isFast for all pre-r317488 bitcode file (remember the afn bit is not set here)
> 2. a pre-r317488 compiler is going to run any optimization guarded by
unsafeAlgebra for any post-r317488 bitcode file that has the reassoc bit
(remember we repurposed UnsafeMath)
> 
> Scenario #2 might be unlikely but we’re potentially breaking the semantic
of the program. It is particularly dangerous because there is nothing that is
going to tell us that we are in this situation “downgrade" situation.
> #1 means that any code that uses unsafeMath is going to get a performance
hit.
> 
> In other words, one scenario implies generating wrong code and the other,
runtime performance regressions.
> 
> 
> 
> Scenario #1 is unsupported AFAIK, unless I missed something the bitcode is
not forward compatible: loading newer bitcode with an older LLVM has never been
supported as far I can remember.
I agree, but we cannot detect that this is the situation we are in and debugging
would prove hard. That said, if we say we don’t care, so be it :).
> 
> Scenario #2 is very much like other performance regression when we drop old
metadata (i.e. bitcode upgrade isn't performance proof in general but only
"best effort", there have been multiple instance of this in the past).
> 
> Usually IIRC we try not to version the bitcode at all this way (i.e.
bitcode does not have a linear versioning that is regularly bumped) but instead
make sure the encoding itself allows an easy upgrade. I.e. the encoding of the
FMF should have been such that the reader can detect and upgrade to the new IR
representation.
> Now this is too late here I guess, so bumping may be a possible trade-off.
Do you think we should do it, or live with the performance drop?
I know this performance drop is not acceptable for our use cases, but I don’t
want to impose our ruling on this.

> What about any bitcode shipped after r317488 but before the version bump?
Not worth taking into account because of the short period of time?
I was hoping we can ignore that period of time because it was not in any
official LLVM release.

Best,
-Quentin
> 
> Best,
> 
> -- 
> Mehdi
>  
> 
> 
> * Feedback Needed *
> 
> I believe this change is big enough that it would be worth bumping the
bitcode version so that the upgrader can do the right thing *before* we release
it to the public with LLVM-6.0.
> 
> That being said, I don’t know what are the implications of such bump and if
people really don’t care about the performance problem that might be okay. The
silent downgrade path is however concerning.
> 
> Should we bump the bitcode version because of that change and have the
autoupgrader properly rewrite the fast-math flags to
> preserve the semantic and make sure there are no silent downgrade?
> 
> 
> Thanks,
> -Quentin
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180213/47a2bd30/attachment.html>