Compile bools to i1 #15005
Compile bools to i1 #15005
Conversation
|
Currently we're at least somewhat compatible with LLVM 3.3 and 3.4, would this break compatibility with both of them? |
|
It would definitely break compatibility with older releases. I don't think it makes sense to support stable releases with master though as it would hold back development. Clang follows along with LLVM trunk and only the stable releases work with the corresponding LLVM stable release. Once we have supported releases, we could easily coordinate it to support the latest stable release of LLVM without freezing the version on trunk for long. |
|
The LLVM update would break compatibility. The SROA fix is (IIRC) for a
|
It should be noted that this will break all travis builds for Rust, and Servo has started to rely on travis infrastructure with a pre-installed LLVM. These can be overcome, I just want to make sure that we're aware of the consequences.
Yes, but this may not necessarily need to hold back development. In theory an older rustc could detect that it's using and older LLVM and fall back to using i8 instead of i1. It would be annoying in some places, but it is not outside the realm of possibility.
Ok, but our automation is very different, and has infrastructure has different needs. As I noted with the travis builds above, it is quite nice to be able to use a pre-installed LLVM for a variety of reasons. Again though, it is not a requirement for us to be compatible with older LLVM, I want to make it clear that we are relying on older LLVM builds in numerous places today to understand what will happen if we break them. I also want to make it clear that supporting an older LLVM release doesn't mean holding off on all newer features of LLVM in the later versions. |
Installing a compatible version of LLVM in Travis is still possible. You could do a single build of LLVM, archive or package it and then make it available on rust-lang.org. Perhaps the snapshot builders could be responsible for doing this, but if it really still works as far back as 3.3 right now then it won't be hard to do it manually as-needed. |
|
(Pardon the noob question) Would this mean I could transmute into a struct like: #[packed] struct Msg { a: bool, b: bool, c: bool, /* ... */, h: bool }from a bitfield stored as a |
No, the datalayout still specifies i1 to take up a whole byte, even when used in a packed struct. |
|
Just verified that the bug in the SROA pass that caused miscompilations with bool as i1 is not present in LLVM 3.4. Shall I cherry-pick the bugfix onto the LLVM version that's currently used by rust, submit a PR against our llvm fork and update this PR accordingly? |
Oh! Awesome! So this means that if we compile bool as i1, then LLVM 3.4 will continue to work?
I'm a little confused, didn't you already do that? (and include it as part of this PR) |
I updated LLVM to a version that includes the bugfix, no need to cherry-pick it. What I meant here was that instead of using a newer upstream, I'd backport only the bugfix commit. The reason I thought of that was because of the cmpxchg change that is the reason why the update would break compatibility. But like you said, trans could actually just check which type cmpxchg has and translate the intrinsics that use it accordingly. |
|
Ah, I see. Updating LLVM is fine, we've already got a good bit of 3.5-specific code (especially around cmpxchg). Do you know if this breaks 3.3 compatibility? To make progress on this patch, I see that travis failed because it's building with 3.4 instead of 3.5. Could you use appropriate |
|
Depends on rust-lang/llvm#9 |
|
Depends on rust-lang/llvm#11 now |
The ArgType type gives us a generic way to specify an attribute for a type to ensure ABI conformance for foreign functions. But the code that actually sets the argument attributes in the function declaration only sets the attribute for the return type when the type is indirect. Since LLVMAddAttribute() doesn't allow to set attributes on the return type, we have to use LLVMAddFunctionAttribute() instead. This didn't cause problems yet, because currently only some indirect types require attributes to be set.
When calling a foreign function, some arguments and/or return value attributes are required to conform to the foreign ABI. Currently those attributes are only added to the declaration of foreign functions. With direct calls, this is no problem, because LLVM can see that those attributes apply to the call. But with an indirect call, LLVM cannot do that and the attribute is missing. To fix that, we have to add those attribute to the calls to foreign functions as well. This also allows to remove the special handling of the SRet attribute, which is ABI-dependent and will be set via the `attr` field of the return type's `ArgType`.
To fix rust-lang#8106, we need an LLVM version that contains r211082 aka 0dee6756 which fixes a bug that blocks that issue. There have been some tiny API changes in LLVM, and cmpxchg changed its return type. The i1 part of the new return type is only interesting when using the new weak cmpxchg, which we don't do.
We currently compiled bools to i8 values, because there was a bug in LLVM that sometimes caused miscompilations when using i1 in, for example, structs. Using i8 means a lot of unnecessary zero-extend and truncate operations though, since we have to convert the value from and to i1 when using for example icmp or br instructions. Besides the unnecessary overhead caused by this, it also sometimes made LLVM miss some optimizations. Fixes rust-lang#8106.
I disagree. We should keep compatibility with at least the current stable. We don't control llvm's developement cycles and we can't make Rust depend of those. if we release a Rust verison that is not compatible with the current llvm stable release, we'll be in trouble. Ideally, we should support LLVM's version shipped in LTS distro versions. IMHO. |
|
@flaper87: The upstream recommendation is generally to follow trunk as |
|
@thestinger I don't think it makes no sense. It really boils down to how much we want to keep backwards compatibility. I still think supporting at least the latest stable is important. |
|
@flaper87: Why is it important? It's not something offered by |
We currently compiled bools to i8 values, because there was a bug in LLVM that sometimes caused miscompilations when using i1 in, for example, structs. Using i8 means a lot of unnecessary zero-extend and truncate operations though, since we have to convert the value from and to i1 when using for example icmp or br instructions. Besides the unnecessary overhead caused by this, it also sometimes made LLVM miss some optimizations. First, we have to fix some bugs concerning the handling of attributes in foreign function declarations and calls. These are required because the i1 type needs the ZExt attribute when used as a function parameter or return type. Then we have to update LLVM to get a bugfix without which LLVM sometimes generates broken code when using i1. And then, finally, we can switch bools over to i1.
|
Looks like this had a significant impact on compile times: http://huonw.github.io/isrustfastyet/buildbot/ |
|
I suspect this is because it includes an LLVM update. Normally the buildbot doesn't rebuild LLVM. |
|
Yeah, it's back down to normal after the LLVM rebuild. |
…nicola Fix typo in reload.rs Thanks for the amazing LSP! I found a small typo and fixed it.
We currently compiled bools to i8 values, because there was a bug in
LLVM that sometimes caused miscompilations when using i1 in, for
example, structs.
Using i8 means a lot of unnecessary zero-extend and truncate operations
though, since we have to convert the value from and to i1 when using for
example icmp or br instructions. Besides the unnecessary overhead caused
by this, it also sometimes made LLVM miss some optimizations.
First, we have to fix some bugs concerning the handling of
attributes in foreign function declarations and calls. These
are required because the i1 type needs the ZExt attribute when
used as a function parameter or return type.
Then we have to update LLVM to get a bugfix without which LLVM
sometimes generates broken code when using i1.
And then, finally, we can switch bools over to i1.