Skip to content
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

Discriminants may not want to all be as small as possible #11551

Closed
alexcrichton opened this issue Jan 15, 2014 · 8 comments
Closed

Discriminants may not want to all be as small as possible #11551

alexcrichton opened this issue Jan 15, 2014 · 8 comments
Labels
A-codegen Area: Code generation

Comments

@alexcrichton
Copy link
Member

If I have a struct Option<uint>, the way that we represent this today is via struct { u8 tag; uint value; }. This is ideal for discriminant size, and even for struct size. The bad part is that we add explicit padding (7 bytes on 64-bit machines) to make up for the middle padding. This leads to horrible things like unaligned moves in LLVM and alloca's of 7 bytes.

I believe that it may actually be better for a discriminant to be the smallest size above or equal to the maximum alignment of the struct. So in the above case the discriminant would actually become uint, and for Option<u8> it would remain u8. This would remove any padding between the discriminant and the beginning of the data.

cc @jld, I think this would remove some need for tbaa.struct for now, although we'll still want that eventually. I'm also not sure if reading a word is more efficient than reading a byte, but it kinda seems so to me?

@alexcrichton
Copy link
Member Author

Another idea that @Aatch came up with was to put the discriminant at the end of the struct so we wouldn't have to worry about alignment at all (and we keep the same size calculations that we have today).

@jld
Copy link
Contributor

jld commented Jan 16, 2014

Option<uint> is more like:

union {
  struct { u8 tag; uint value; } Some;
  struct { u8 tag; } None;
}

And similarly for any other general-case enum. I mention this because the nice thing about having the discriminant at the beginning is that we don't have to do anything fancy in the per-variant LLVM types to have it at the right offset.

If we just want to get rid of the { i8, [7 x i8], i64 } thing, could we use { [8 x i8], i64 } instead, and add another 0 to the GEP address of the discriminant? Would LLVM fail to optimize things because of the array indexing involved? Would copying the [8 x i8] be handled like copying ani64 or would LLVM still do something weird?

@alexcrichton
Copy link
Member Author

I've seen LLVM do some nice optimizations in the past of memcpy with size 8 becoming a load of a 64-bit value, so I think that changing to [i8 x 8] would also help this. The true solution is to use tbaa.struct to tell LLVM that it doesn't need to copy the intermediary.

If this is an involved solution, then it's not really worth it to pursue. Out of curiosity, is there a reason for Option<uint> to not have a uint discriminant? I figured we'd just change some calculation somewhere to put the lower bound of the discriminant size at the maximum alignment and then everything else would be fixed, but I'm also not familiar with the code so I'm not sure how hard that would be.

@dotdash
Copy link
Contributor

dotdash commented Jan 18, 2014

If Option<uint> was like this:

struct {
    union {
        uint value;
    }
    u8 tag;
}

Wouldn't that get avoid the alignment issue without requiring special handling for each variant? Basically the LLVM type would change from { i8, [7 x i8], [1 x i64] } to { [1 x i64], i8 }, which seems easier overall. Just passing 1 instead of 0 to GEP. Am I missing anything?

@thestinger
Copy link
Contributor

@dotdash: You're missing that struct { i64, i8 } needs to have padding in order to maintain the correct alignment for i64 in an array. Rust is responsible for handling padding in the frontend.

@dotdash
Copy link
Contributor

dotdash commented Jan 18, 2014

@thestinger: Ah right, completely lost track of arrays somewhere in the middle of my thoughts.

@huonw
Copy link
Member

huonw commented Jan 26, 2014

cc #6736 for tbaa.struct.

@alexcrichton
Copy link
Member Author

I've been led to believe that this isn't as easy a solution as I thought, and in some cases this may make structs larger. I'm going to close in favor of the correct solution, #6736

flip1995 pushed a commit to flip1995/rust that referenced this issue Sep 25, 2023
…t, r=xFrednet

fixed fp caused by moving &mut reference inside of a closure

changelog: [`needless_pass_by_ref mut`]: fixes false positive caused by not covering mutable references passed to a closure inside of a fuction
fixes rust-lang#11545
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation
Projects
None yet
Development

No branches or pull requests

5 participants