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

[Sanjay Patel via llvm-dev]

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>