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

[via llvm-dev]

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>