New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unfortunate optimization with Option<bool> vs Option<uint> #20149
Comments
Seems to happen for all integer types smaller than int/uint |
Noticable difference is that the store %"enum.core::option::Option<[bool]>[#3]" { i8 1, [0 x i8] undef, [1 x i8] c"\01" }, %"enum.core::option::Option<[bool]>[#3]"* %0, align 1 And I guess the pass responsible for optimizing the icmp away can't handle that. Since we originally emit independent stores, I guess this should be handled in LLVM upstream. |
OTOH I can't reproduce the problem with clang. So maybe we could do something about the way unions are represented in LLVM types. |
OK, it's something completely different. The assignment happens via |
Because |
There is a comment in expr.rs that seems suspicious. Commenting out the line causes Maybe whoever wrote the comment could chime in? The library split has made it difficult to find out who though. |
This commit added the comment in question: ee4ba449 @nikomatsakis can you confirm that it was related to segmented stacks? |
http://reviews.llvm.org/D6829 -- This teaches LLVM to handle the aggregate and extract the loaded value from it, allowing it to optimize the code even without #20161. It also handles the example I presented there where the deferred load breaks an optimization. |
Wow, thanks so much for looking into this @dotdash! |
Currently, small aggregates are passed to functions as immediate values as is. This has two consequences. One is that aggregates are passed component-wise by LLVM, so e.g. a struct containing four u8 values (e.g. an RGBA struct) will be passed as four individual values. The other is that LLVM isn't very good at optimizing loads/stores of first class attributes. What clang does is converting the aggregate to an appropriately sized integer type (e.g. i32 for the four u8 values), and using that for the function argument. This allows LLVM to create code that is a lot better. Fixes rust-lang#20450 rust-lang#20149 rust-lang#16506 rust-lang#13927
Currently, small aggregates are passed to functions as immediate values as is. This has two consequences. One is that aggregates are passed component-wise by LLVM, so e.g. a struct containing four u8 values (e.g. an RGBA struct) will be passed as four individual values. The other is that LLVM isn't very good at optimizing loads/stores of first class attributes. What clang does is converting the aggregate to an appropriately sized integer type (e.g. i32 for the four u8 values), and using that for the function argument. This allows LLVM to create code that is a lot better. Fixes #20450 #20149 #16506 #13927
Currently, small aggregates are passed to functions as immediate values as is. This has two consequences. One is that aggregates are passed component-wise by LLVM, so e.g. a struct containing four u8 values (e.g. an RGBA struct) will be passed as four individual values. The other is that LLVM isn't very good at optimizing loads/stores of first class attributes. What clang does is converting the aggregate to an appropriately sized integer type (e.g. i32 for the four u8 values), and using that for the function argument. This allows LLVM to create code that is a lot better. Fixes #20450 #20149 #16506 #13927
Closed by #20755. |
Currently, given this program:
It generates this IR.
The "unfortunate" part here is that a call to failure is generated for the
get_bool
function where no failure is generated for theget_uint
function.There's a lot going on here so I'm not sure if it's our codegen or LLVM's codegen, but I would personally expect both cases to optimize down to never unwinding because it's known that neither
Option
can ever beNone
.The text was updated successfully, but these errors were encountered: