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

2018-02-13 17:46 GMT-08:00 Mehdi AMINI <joker.eph at gmail.com>:
>
>
> 2018-02-13 13:29 GMT-08:00 Quentin Colombet <qcolombet at apple.com>:
>
>> 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>:
>>
>>> 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.
>>
>>
>> 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 :).
>>
>
> I just mean that in general many things can blow up when the bitcode
> evolves, and we don't expect or support (as far as I know) llvm-3.9 to
be
> used to read/process bitcode from llvm-4.0 for instance.
> (I remember implementing in the Apple LLVM a hard check and failure for
> this: we don't even try)
>
>
>
>>
>>
>> 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'm not saying this shouldn't be done, I don't have a dog in
this fight: I
> was just providing another data point "as a matter of general
policy" in
> the past there have been other cases where we haven't been able (or
rather
> chose to not put effort) into preserving all the performance when upgrading
> (I think TBAA comes to my mind right now, but other kind of metadata have
> also been lost). Now every case is different, and the FMF can be considered
> too important to be left on the table.
> For example it would impact LTO when loading libraries built with Xcode9
> and before whenever the next released post-r317488 come up.
>
>
>
>> 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.
>>
>
Need to at least make sure everyone knows what to expect, so the
cherry-pick for the LLVM 6.0 release should include an entry in the Release
Notes.

Hans: this can only happen if you're still OK to cherry-pick in LLVM 6.0
branch https://reviews.llvm.org/D43253

Duncan, Steven, Peter, Teresa: this change can break LTO if we read back
bitcode emitted from trunk since r317488 (last November), any concerns?

Best,

-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180216/1d83c8f5/attachment.html>

On Fri, Feb 16, 2018 at 8:26 AM, Mehdi AMINI <joker.eph at gmail.com>
wrote:
>
>
> 2018-02-13 17:46 GMT-08:00 Mehdi AMINI <joker.eph at gmail.com>:
>
>>
>>
>> 2018-02-13 13:29 GMT-08:00 Quentin Colombet <qcolombet at
apple.com>:
>>
>>> 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>:
>>>
>>>> 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.
>>>
>>>
>>> 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 :).
>>>
>>
>> I just mean that in general many things can blow up when the bitcode
>> evolves, and we don't expect or support (as far as I know) llvm-3.9
to be
>> used to read/process bitcode from llvm-4.0 for instance.
>> (I remember implementing in the Apple LLVM a hard check and failure for
>> this: we don't even try)
>>
>>
>>
>>>
>>>
>>> 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'm not saying this shouldn't be done, I don't have a dog
in this fight:
>> I was just providing another data point "as a matter of general
policy" in
>> the past there have been other cases where we haven't been able (or
rather
>> chose to not put effort) into preserving all the performance when
upgrading
>> (I think TBAA comes to my mind right now, but other kind of metadata
have
>> also been lost). Now every case is different, and the FMF can be
considered
>> too important to be left on the table.
>> For example it would impact LTO when loading libraries built with
Xcode9
>> and before whenever the next released post-r317488 come up.
>>
>>
>>
>>> 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.
>>>
>>
> Need to at least make sure everyone knows what to expect, so the
> cherry-pick for the LLVM 6.0 release should include an entry in the Release
> Notes.
>
> Hans: this can only happen if you're still OK to cherry-pick in LLVM
6.0
> branch https://reviews.llvm.org/D43253
>
> Duncan, Steven, Peter, Teresa: this change can break LTO if we read back
> bitcode emitted from trunk since r317488 (last November), any concerns?
>
For our usage of ThinLTO, we don't reuse bitcode across different
revisions, so it wouldn't cause an issue. The LTO caching in
lib/LTO/LTO.cpp also encodes the llvm revision, so it shouldn't be an issue
there either.

Teresa

> Best,
>
> --
> Mehdi
>
>
>
>
>

-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180216/251ec089/attachment.html>

Thanks to all a better fix landed on both trunk and LLVM 6.0!
> On Feb 16, 2018, at 11:37 AM, Teresa Johnson <tejohnson at
google.com> wrote:
> 
> 
> 
> On Fri, Feb 16, 2018 at 8:26 AM, Mehdi AMINI <joker.eph at gmail.com
<mailto:joker.eph at gmail.com>> wrote:
> 
> 
> 2018-02-13 17:46 GMT-08:00 Mehdi AMINI <joker.eph at gmail.com
<mailto:joker.eph at gmail.com>>:
> 
> 
> 2018-02-13 13:29 GMT-08:00 Quentin Colombet <qcolombet at apple.com
<mailto:qcolombet at apple.com>>:
> Hi Mehdi,
> 
> 
>> On Feb 13, 2018, at 12:34 PM, Mehdi AMINI <joker.eph at gmail.com
<mailto: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 :).
> 
> I just mean that in general many things can blow up when the bitcode
evolves, and we don't expect or support (as far as I know) llvm-3.9 to be
used to read/process bitcode from llvm-4.0 for instance.
> (I remember implementing in the Apple LLVM a hard check and failure for
this: we don't even try)
> 
>  
> 
>> 
>> 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'm not saying this shouldn't be done, I don't have a dog in
this fight: I was just providing another data point "as a matter of general
policy" in the past there have been other cases where we haven't been
able (or rather chose to not put effort) into preserving all the performance
when upgrading (I think TBAA comes to my mind right now, but other kind of
metadata have also been lost). Now every case is different, and the FMF can be
considered too important to be left on the table.
> For example it would impact LTO when loading libraries built with Xcode9
and before whenever the next released post-r317488 come up.
> 
>  
> 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.
> 
> Need to at least make sure everyone knows what to expect, so the
cherry-pick for the LLVM 6.0 release should include an entry in the Release
Notes.
> 
> Hans: this can only happen if you're still OK to cherry-pick in LLVM
6.0 branch https://reviews.llvm.org/D43253
<https://reviews.llvm.org/D43253>
> 
> Duncan, Steven, Peter, Teresa: this change can break LTO if we read back
bitcode emitted from trunk since r317488 (last November), any concerns?
> 
> For our usage of ThinLTO, we don't reuse bitcode across different
revisions, so it wouldn't cause an issue. The LTO caching in lib/LTO/LTO.cpp
also encodes the llvm revision, so it shouldn't be an issue there either.
> 
> Teresa
> 
> 
> Best,
> 
> -- 
> Mehdi
> 
> 
> 
> 
> 
> 
> 
> -- 
> Teresa Johnson |	 Software Engineer |	 tejohnson at google.com
<mailto:tejohnson at google.com> |	 408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180226/a8defc6f/attachment-0001.html>