Page MenuHomePhabricator

bitcode support change for fast flags compatibility
ClosedPublic

Authored by mcberg2017 on Feb 13 2018, 12:18 PM.

Details

Summary

The discussion and as per need, each vendor needs a way to keep the old fast flags and the new fast flags in the auto upgrade path of the IR upgrader. This revision addresses that issue.

Diff Detail

Repository
rL LLVM

Event Timeline

mcberg2017 created this revision.Feb 13 2018, 12:18 PM
This comment was removed by mcberg2017.
mcberg2017 added a comment.EditedFeb 13 2018, 12:26 PM

Uploaded above are the 6 bitcode files we will need to replace for check to run successfully as these files had the old version number in them.

Uploaded above are the 6 bitcode files we will need to replace for check to run successfully as these files had the old version number in them.

Weren't these files generated with the corresponding version of LLVM? We should check that they upgrade correctly, not prevent them from upgrading I believe.

mehdi_amini added inline comments.Feb 13 2018, 1:03 PM
include/llvm/IR/Module.h
189 ↗(On Diff #134095)

Why does it make sense to have on the Module?

lib/Bitcode/Reader/BitcodeReader.cpp
426 ↗(On Diff #134095)

I'm not familiar with this record, why use this instead of bitc::BITCODE_CURRENT_EPOCH?

For the 6 bitcode files, they are special context, as they require the current fast sub flags format for part of their use, so we have to bring them up into the current context as they require the use of the new flags selectively with the bumped version number.

mcberg2017 added inline comments.Feb 13 2018, 1:18 PM
include/llvm/IR/Module.h
189 ↗(On Diff #134095)

We had it before, we just never held the value in storage (we only checked it), and I believe (someone correct me if I misspeak), this is where we bumped the bitcode version.

For the 6 bitcode files, they are special context, as they require the current fast sub flags format for part of their use, so we have to bring them up into the current context as they require the use of the new flags selectively with the bumped version number.

Can you elaborate what you mean by "they require the current fast sub flags format for part of their use"?

include/llvm/IR/Module.h
189 ↗(On Diff #134095)

That's not answering why it makes sense to add it here, and now.

mehdi_amini added inline comments.Feb 13 2018, 9:36 PM
lib/Bitcode/Reader/BitcodeReader.cpp
426 ↗(On Diff #134095)

just to make you didn't miss this: I don't know why bumping ModuleVersion instead of bitc::BITCODE_CURRENT_EPOCH?

Actually if we're making this change, we consider that bitcode generated between the introduction of the new fast-math flags and now isn't supported anymore.
If so then we could instead of bumping a version, change the encoding of the FMF record (or create a new record type), so that the old encoding is upgraded but not the new one.
That would seem to me to be more in line with the usual handling of such situation in the bitcode.

mcberg2017 added inline comments.Feb 14 2018, 12:10 PM
include/llvm/IR/Module.h
189 ↗(On Diff #134095)

I'll attempt to answer both comments here (lines 189/426): My interpretation of {b738d340, the review for adding EPOCH and other components} is that with IDENTIFICATION_CODE_VERSION removed and major and minor version number are no longer maintained and with no vehicle comparing LLVM_VERSION_MAJOR/LLVM_VERSION_MINOR in this context for bitcode versioning that the action was open to interpretation for a plausible solution. Subsequently that using the Version Record and giving it storage would work well as it is already overloaded to modify actions regarding what and how to read bitcode.

mehdi_amini added inline comments.Feb 14 2018, 8:35 PM
include/llvm/IR/Module.h
189 ↗(On Diff #134095)

What is b738d340? Can you use the SVN rev please?

I don't understand your answer, like almost anything. I'll try to clarify what I'm asking:

  1. At the bitcode level why is EPOCH not the right thing to use?
  2. Even if you were to use the module version record in the bitcode. That does not justify by itself to expose it on the IR structure that is the class Module. I'm missing the motivation to change anything outside of IR.

(also, this discussion is unnecessary if we follow my other suggestion which is to change the encoding of the FMF record).

mcberg2017 added inline comments.Feb 15 2018, 10:03 AM
include/llvm/IR/Module.h
189 ↗(On Diff #134095)

The comment above pertains to this review: https://reviews.llvm.org/D13666

and this svn rev: https://gitlab.kitware.com/third-party/llvm/commit/b738d340fa9958dbefe8d23a4d4659c2255b823a

Changing the encoding format for the FMF record is a non starter.

mehdi_amini added inline comments.Feb 15 2018, 10:15 AM
include/llvm/IR/Module.h
189 ↗(On Diff #134095)

Changing the encoding format for the FMF record is a non starter.

Why?

mcberg2017 added inline comments.Feb 15 2018, 10:50 AM
include/llvm/IR/Module.h
189 ↗(On Diff #134095)

First lets step back a bit.

If one objection is the use of storage in Module.h, then I can carry the state around in the reader.

By using EPOCH do you mean changing the value from 0 to 1 to interpret the effective mapping of the fast math flags as old verses new?

mehdi_amini added inline comments.Feb 15 2018, 11:17 AM
include/llvm/IR/Module.h
189 ↗(On Diff #134095)

First lets step back a bit.

That's what I did and lead me to look at the FMF encoding instead.
The FMF is a uint64_t, there is likely a bit that could be used for versioning here, or other means of encoding that would help with the upgrade.

By using EPOCH do you mean changing the value from 0 to 1 to interpret the effective mapping of the fast math flags as old verses new?

I looked back at EPOCH and it won't help: it is not designed to help with upgrade, it is designed as a barrier for not to read an old bitcode entirely.

I quickly put a patch to change the encoding, may be easier to discuss with code at hand: https://reviews.llvm.org/D43348
(I didn't try to build it, just a quick draft)

For the 6 bitcode files, they are special context, as they require the current fast sub flags format for part of their use, so we have to bring them up into the current context as they require the use of the new flags selectively with the bumped version number.

Can you elaborate what you mean by "they require the current fast sub flags format for part of their use"?

Ping on this.

So the compatibility .bc files in check test have usages of the current flag layout with reassoc, afn and the new layout, as has been the case since last Nov. These tests are not old fast format but sub flags tests in the new format, and require the new mapping.

So the compatibility .bc files in check test have usages of the current flag layout with reassoc, afn and the new layout, as has been the case since last Nov. These tests are not old fast format but sub flags tests in the new format, and require the new mapping.

Please be very specific and provide example of what is broken in the test/Bitcode/compatibility-3.6.ll for example.

Case in point:

./bin/llvm-dis < /Users/michael_c_berg/llvm/llvm/test/Bitcode/compatibility-3.6.ll.bc | ./bin/FileCheck ../llvm/test/Bitcode/compatibility-3.6.ll
../llvm/test/Bitcode/compatibility-3.6.ll:617:11: error: expected string not found in input
; CHECK: %f.fast = fadd reassoc nnan ninf nsz arcp float %op1, %op2

^

<stdin>:445:2: note: scanning from here
%f.fast = fadd fast float %op1, %op2

in this case we are using my current patch with module version at 2 in the key .bc file, however we need the mapping as:

reassoc
nnan
ninf
nsz
arch
contract
afn

the new format writes reassoc in position 0 and aliases to UnsafeAlgebra incorrectly in this case, causing us to map fast, when less than fast was specified in the IR.
This will still be the case with the other format.
^

Case in point:

./bin/llvm-dis < /Users/michael_c_berg/llvm/llvm/test/Bitcode/compatibility-3.6.ll.bc | ./bin/FileCheck ../llvm/test/Bitcode/compatibility-3.6.ll
../llvm/test/Bitcode/compatibility-3.6.ll:617:11: error: expected string not found in input

; CHECK: %f.fast = fadd reassoc nnan ninf nsz arcp float %op1, %op2
         ^

<stdin>:445:2: note: scanning from here

%f.fast = fadd fast float %op1, %op2

in this case we are using my current patch with module version at 2 in the key .bc file, however we need the mapping as:

reassoc
nnan
ninf
nsz
arch
contract
afn

the new format writes reassoc in position 0 and aliases to UnsafeAlgebra incorrectly in this case, causing us to map fast, when less than fast was specified in the IR.
This will still be the case with the other format.

^

Thanks for being specific.
The right fix is to change the check line, not the bitcode.

Strongly disagree. The new test's purpose is to provide functional testing on the new flags in the new format. Sanjay, now would be a good time to chime in.

Strongly disagree. The new test's purpose is to provide functional testing on the new flags in the new format. Sanjay, now would be a good time to chime in.

We don't have the same understanding I believe: my take on this is that this test is supposed to ensure that LLVM upgrade correctly old bitcode. So changing the bitcode is a huge red flag to begin with,
The CHECK line on the other is intended to be adjusted: it is testing what is the current output of LLVM for an old bitcode.

So the question is: for the bitcode generated by LLVM 3.6 with a "fast" flag. What do you want printed by LLVM today?

Ping @vsk as well.

vsk added a comment.Feb 15 2018, 1:54 PM

Strongly disagree. The new test's purpose is to provide functional testing on the new flags in the new format. Sanjay, now would be a good time to chime in.

We don't have the same understanding I believe: my take on this is that this test is supposed to ensure that LLVM upgrade correctly old bitcode. So changing the bitcode is a huge red flag to begin with,
The CHECK line on the other is intended to be adjusted: it is testing what is the current output of LLVM for an old bitcode.

+ 1, yes exactly. The point of checking in the bitcode generated by the 3.6 compiler is that future compilers must understand it gracefully :). This is fundamental for allowing submission of bitcode to Apple's app store and is a documented requirement: https://llvm.org/docs/DeveloperPolicy.html#ir-backwards-compatibility.

So the question is: for the bitcode generated by LLVM 3.6 with a "fast" flag. What do you want printed by LLVM today?

Yep. It's fine if new compilers change the semantics of old bitcode (within reason), but the old bitcode must be handled gracefully. We need to continue testing that: i.e we can't replace the old bitcode files, but it's OK to update the check lines in the *.ll file.

Strongly disagree. The new test's purpose is to provide functional testing on the new flags in the new format. Sanjay, now would be a good time to chime in.

We don't have the same understanding I believe: my take on this is that this test is supposed to ensure that LLVM upgrade correctly old bitcode. So changing the bitcode is a huge red flag to begin with,
The CHECK line on the other is intended to be adjusted: it is testing what is the current output of LLVM for an old bitcode.

So the question is: for the bitcode generated by LLVM 3.6 with a "fast" flag. What do you want printed by LLVM today?

I haven't commented because I don't have any expertise on the trade-offs as far as the implementation goes, but IIUC the point of this exercise is to get old bitcode that was 'fast' to be 'fast' again. So we want the CHECK lines in the compatibility files that were changed in D39304 to go back to being 'fast'. Ie, we want to reverse the diffs anywhere there's a comment like:

; 'fast' used to be its own bit, but this changed in Oct 2017.

As per feedback I have updated the bitcode reader/writer functionality as per Medhi's feedback along with the compatibility tests as per Sanjay's feedback to fast, and corrected the fast map values so that we get unique flag selection, as the code from D43348 would not have worked without shift values. Also I added a change to the fast math encoding size field from 7 to 8 bits. All works as advertised in the check tests.

spatel added inline comments.Feb 16 2018, 6:28 AM
lib/Bitcode/Writer/BitcodeWriter.cpp
3199 ↗(On Diff #134545)

I don't know this code, so I'm curious how this works, and it might be worth a code comment.

What allows us to increase the width of the field? Are we relying on padding bits? If not, how does this work when a new compiler reads an old IR file with the old smaller field?

test/Bitcode/compatibility-3.6.ll
615–616 ↗(On Diff #134545)

Here and everywhere else the comment is repeated: delete it because it's no longer valid.

@hans : this needs to go in LLVM 6.0, is it OK?

lib/Bitcode/Writer/BitcodeWriter.cpp
3199 ↗(On Diff #134545)

The bitcode format is described here: http://www.llvm.org/docs/BitCodeFormat.html

The abbreviation describe how the records will be stored and the abbrev is itself emitted. So that the reader knows how to decode. When a new llvm reads an old bitcode it'll see an abbrev and know that only 7 bits needs to be read. In any case the API unpack and returns a uint64.
Does it make sense?

spatel added inline comments.Feb 16 2018, 8:30 AM
lib/Bitcode/Writer/BitcodeWriter.cpp
3199 ↗(On Diff #134545)

Ah, yes - the size itself is encoded in the binary. Thanks.

dexonsmith added a subscriber: dexonsmith.

Comments removed per recommendation.

Comments removed per recommendation.

Thanks. I don't have any other comments, but someone with a better understanding and a stake in this should give approval.

I think this approach is much better. Can you update the title because it is not really a version bump anymore? From my understanding, this is good enough for backwards-compatibility. Thanks!

I don't know enough about fast math flag to fully approve this. @qcolombet, what do you think?

mcberg2017 retitled this revision from bump the bitcode version for fast flags compatibility to bitcode support change for fast flags compatibility.Feb 16 2018, 10:21 AM
vsk added a comment.Feb 16 2018, 10:36 AM

Thanks, this is looking good! It'd be great to hear Quentin chime in.

qcolombet accepted this revision.Feb 16 2018, 11:01 AM

Thanks all for the great feedbacks!

The final solution is much better than a dumb bump of the bitcode version.

LGTM.

This revision is now accepted and ready to land.Feb 16 2018, 11:01 AM

Thanks all for the great feedbacks!

The final solution is much better than a dumb bump of the bitcode version.

LGTM.

Note: we need buy-in from LLVM 6.0 release manager (@hans ?) that this will be cherry-picked there before landing this in trunk.

hans added a comment.Feb 19 2018, 5:25 AM

Thanks all for the great feedbacks!

The final solution is much better than a dumb bump of the bitcode version.

LGTM.

Note: we need buy-in from LLVM 6.0 release manager (@hans ?) that this will be cherry-picked there before landing this in trunk.

This seems fine for 6.0 (it needs to land soon though), but why does it need to be cherry-picked to the branch before landing on trunk?

steven_wu accepted this revision.Feb 19 2018, 11:15 AM

Michael doesn't have commit right yet so I will commit this for him.

This can cherry-pick after landing on trunk.

This revision was automatically updated to reflect the committed changes.

Thanks all for the great feedbacks!

The final solution is much better than a dumb bump of the bitcode version.

LGTM.

Note: we need buy-in from LLVM 6.0 release manager (@hans ?) that this will be cherry-picked there before landing this in trunk.

This seems fine for 6.0 (it needs to land soon though), but why does it need to be cherry-picked to the branch before landing on trunk?

Sorry for the ambiguity, I meant that we needed your confirmation before landing this in trunk, not that the cherry-pick has to happen before.
i.e. parenthesis were intended like this "we need (buy-in from LLVM 6.0 release manager that this will be cherry-picked there) before landing this in trunk."

Thanks!

hans added a comment.Feb 20 2018, 1:07 AM

Note: we need buy-in from LLVM 6.0 release manager (@hans ?) that this will be cherry-picked there before landing this in trunk.

This seems fine for 6.0 (it needs to land soon though), but why does it need to be cherry-picked to the branch before landing on trunk?

Sorry for the ambiguity, I meant that we needed your confirmation before landing this in trunk, not that the cherry-pick has to happen before.
i.e. parenthesis were intended like this "we need (buy-in from LLVM 6.0 release manager that this will be cherry-picked there) before landing this in trunk."

Makes sense, thanks :-)

I'll let this sit in trunk a few hours more and will merge it later today.

hans added a comment.Feb 20 2018, 8:24 AM

Note: we need buy-in from LLVM 6.0 release manager (@hans ?) that this will be cherry-picked there before landing this in trunk.

This seems fine for 6.0 (it needs to land soon though), but why does it need to be cherry-picked to the branch before landing on trunk?

Sorry for the ambiguity, I meant that we needed your confirmation before landing this in trunk, not that the cherry-pick has to happen before.
i.e. parenthesis were intended like this "we need (buy-in from LLVM 6.0 release manager that this will be cherry-picked there) before landing this in trunk."

Makes sense, thanks :-)

I'll let this sit in trunk a few hours more and will merge it later today.

Merged to 6.0 in r325592.