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

-Calign-jumps=1 support in rustc #128831

Open
ojeda opened this issue Aug 8, 2024 · 11 comments
Open

-Calign-jumps=1 support in rustc #128831

ojeda opened this issue Aug 8, 2024 · 11 comments
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-rust-for-linux Relevant for the Rust-for-Linux project C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ojeda
Copy link
Contributor

ojeda commented Aug 8, 2024

i.e. the equivalent of -falign-jumps=1 (GCC, Clang does not support it).

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 8, 2024
@ojeda
Copy link
Contributor Author

ojeda commented Aug 8, 2024

@rustbot label I-rust-for-linux

@rustbot rustbot added the A-rust-for-linux Relevant for the Rust-for-Linux project label Aug 8, 2024
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 8, 2024
@workingjubilee
Copy link
Member

@ojeda Please do not use I- labels for things that are not relevant for determining the actual impact of an issue with respect to the compiler. There is not an I-amazon nor an I-google nor an I-microsoft on the issue tracker.

With respect to codegen features, you should link to the relevant LLVM feature support. If Clang does not support it, it is likely LLVM does not. If it does not, then this is an LLVM bug.

@workingjubilee workingjubilee added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-codegen Area: Code generation labels Aug 8, 2024
@ojeda
Copy link
Contributor Author

ojeda commented Aug 9, 2024

Please do not use I- labels for things that are not relevant for determining the actual impact of an issue with respect to the compiler. There is not an I-amazon nor an I-google nor an I-microsoft on the issue tracker.

The label was created some days ago by @joshtriplett (thanks!) and if I remember correctly from the meeting, I- was used so that anybody could easily tag things relevant to Rust for Linux due to label permissions (I see A-* also in allow-unauthenticated, so I guess that would also work (?)).

If there is an issue with that (e.g. another label should be used), then please let us know, because we were about to start tagging the issues we have in our lists.

With respect to codegen features, you should link to the relevant LLVM feature support. If Clang does not support it, it is likely LLVM does not. If it does not, then this is an LLVM bug.

rustc supports several backends, so I am not sure what you mean -- do you mean you have a policy of only implementing things LLVM supports?

In addition, generally speaking, even if a given backend does not support a particular feature directly, it could be the case that rustc is able to provide a feature even if LLVM does not provide a convenient way to do it.

@workingjubilee
Copy link
Member

The label was created some days ago by @joshtriplett (thanks!) and if I remember correctly from the meeting, I- was used so that anybody could easily tag things relevant to Rust for Linux due to label permissions (I see A-* also in allow-unauthenticated, so I guess that would also work (?)).

If there is an issue with that (e.g. another label should be used), then please let us know, because we were about to start tagging the issues we have in our lists.

So, using an A-label should be fine. Probably it should be a project/working group and have a PG-* label but whatever, that might not be an unauthenticated label set, and I don't much care about that point. I just know that the I- labels are deliberately a somewhat angry-looking red to draw attention to them.

rustc supports several backends, so I am not sure what you mean -- do you mean you have a policy of only implementing things LLVM supports?

Hm. While hypothetically we can implement something that only "cg_clif" and "cg_gcc" support, it is very difficult to test that something works if our primary codegen backend doesn't implement the necessary prerequisites. Very often, because cg_llvm is more mature, it's the "reference implementation" against which the others are tested. My understanding of how the codegen requirements work here is that it is easy to align function entries with minimal cooperation from the codegen backend, but agonizing to implement that for loops and other jumps without cooperation from the codegen backend.

I suspect you don't want this to be "just a hint" like inlining is, and thus easily disregarded, so it seems likely it is raising our requirements for codegen backends. Not a very tall fence, I would think, as "align jumps to N" is often a pretty small ask and something they have to, to some extent, do anyways, but not on the ground.

...Though, reading the GCC documentation, it's not clear to me what the semantics are, actually, for architectures which require an alignment greater than 1 for jumps, as the text claims that "loops are not aligned" (I assume they mean"jumps") if -falign-jumps=1 is set. However, if your architecture has a min align of 2 or 4 bytes for jumps, then that's infeasible to actually respect. Literally, you cannot encode an unaligned jump, in those cases. Thus alignment is set to an arbitrary number anyways.

Which would suggest that this is, in fact, also a "just a hint" codegen flag? @ojeda Is the intention that the codegen backend is indeed allowed to simply ignore any alignment values it decides are too low and right-size them to a value it prefers?

@workingjubilee
Copy link
Member

workingjubilee commented Aug 11, 2024

And in what universe does someone want to set loop and jump target alignment differently...? On most architectures they aren't even different things...

@ojeda
Copy link
Contributor Author

ojeda commented Aug 14, 2024

So, using an A-label should be fine. Probably it should be a project/working group and have a PG-* label but whatever, that might not be an unauthenticated label set, and I don't much care about that point. I just know that the I- labels are deliberately a somewhat angry-looking red to draw attention to them.

Ah, I see -- I hadn't noticed the color :) For us any prefix is fine really, we normally track things in our lists, but having a tag may be useful for everyone to mark potential things etc.

Hm. While hypothetically we can implement something that only "cg_clif" and "cg_gcc" support, it is very difficult to test that something works if our primary codegen backend doesn't implement the necessary prerequisites. Very often, because cg_llvm is more mature, it's the "reference implementation" against which the others are tested.

That makes sense, yeah. I would imagine it is hard to maintain things that only GCC supports without a mature backend. This is not critical for us at the moment, since we mainly support LLVM builds, but eventually we will want to match C on the GCC builds. By then, perhaps the GCC backend is more mature.

As for the feature itself -- in the kernel it is only used as =1, and only for x86_64 and only if the compiler supports it. Same for -falign-loops. The commit that introduced it has some details: Rust-for-Linux/linux@be6cb02 (and for -falign-loops: Rust-for-Linux/linux@52648e8). Essentially, in the kernel (at least back then) it meant a combined 5.5% reduction in text size.

I assume it is "just an optimization" for GCC, since it does it only for non-cold ones apparently, so it would be hard to rely on anyway. From what I see in those kernel commits, it sounds like the kernel only uses it as an optimization as well. So even if it was a guarantee by GCC for all non-cold ones, I think a hint would work fine for us. Though if a particular backend guarantees a bit more, it would be nice to provide that via that backend (even if the "surface level option" in rustc does not promise it by itself).

As for LLVM, for things that Clang does not implement (yet?) but that may be doable via LLVM, in general, it may be a nice opportunity to provide something better than Clang if they don't expose a particular optimization but rustc does so. But, yeah, for us this flag in the LLVM case it is not a big deal since it is not there in Clang.

Thanks!

@ojeda
Copy link
Contributor Author

ojeda commented Aug 14, 2024

And in what universe does someone want to set loop and jump target alignment differently...? On most architectures they aren't even different things...

I think the distinction may be around what the manual hints at with "for branch targets where the targets can only be reached by jumping", even if they all end up being implemented with jumps. For instance, playing a bit with both options, I can get one flag to apply to a goto but not to a for and vice versa, but both are using j*s. With other examples I tried quickly, it is not clear what counts as a loop, though. It does not appear to be just about backwards/forwards jumps either.

@joshtriplett
Copy link
Member

joshtriplett commented Aug 14, 2024

@workingjubilee I'm the one who created the label, and at the time I was not clear on whether it should be an I-* or A-* or O-* label, so I took a best guess based on other labels I saw. Please do blame me for that and not @ojeda. :)

@joshtriplett
Copy link
Member

It sounds like RfL's use of this is exclusively as a hint for optimization. If we called this hint-align-jumps and defined the semantic to be non-guaranteed, I think it'd be fine to ship this without universal backend support. (Though I do think we should reach out to LLVM about support.)

For future non-optional functionality that's backend-specific, we don't have a general policy. I think we'd need to consider it on a case-by-case basis, and at a minimum error if asking for it on a backend that doesn't support it.

@ojeda
Copy link
Contributor Author

ojeda commented Aug 15, 2024

@workingjubilee I'm the one who created the label, and at the time I was not clear on whether it should be an I-* or A-* or O-* label, so I took a best guess based on other labels I saw. Please do blame me for that and not @ojeda. :)

Thanks @joshtriplett, I appreciate it. It was fine to get some blame in any case, after all, we discussed the label together :)

It sounds like RfL's use of this is exclusively as a hint for optimization. If we called this hint-align-jumps and defined the semantic to be non-guaranteed, I think it'd be fine to ship this without universal backend support. (Though I do think we should reach out to LLVM about support.)

For future non-optional functionality that's backend-specific, we don't have a general policy. I think we'd need to consider it on a case-by-case basis, and at a minimum error if asking for it on a backend that doesn't support it.

Sounds good. For the kernel typical use cases, throwing an error if the backend does not support something would be perfectly fine, since we can tweak flags depending on the compiler, i.e. we don't expect every combination to work (Josh of course knows this -- just adding some context here for others). In fact, for non-optional functionality, I think anything else would be quite surprising/dangerous (at least by default -- perhaps there could be a way to say "it is fine to ignore these particular flags if they do not work for the current backend" in rustc, but at least for the kernel it is common to test for flags etc. anyway).

@workingjubilee
Copy link
Member

workingjubilee commented Sep 11, 2024

I assume it is "just an optimization" for GCC, since it does it only for non-cold ones apparently, so it would be hard to rely on anyway. From what I see in those kernel commits, it sounds like the kernel only uses it as an optimization as well. So even if it was a guarantee by GCC for all non-cold ones, I think a hint would work fine for us. Though if a particular backend guarantees a bit more, it would be nice to provide that via that backend (even if the "surface level option" in rustc does not promise it by itself).

Yeah, generally we try to pass on expectations to the backend faithfully and generally the backend upholds those. #[inline(always)] for instance does in fact almost always inline the code... often if it's a bad idea which is why it can even be a deoptimization. Any "just a hint" disclaimers tend to cover for "...wait, no, that only makes sense 99% of the time, so we're just going to ignore it in this case", either on our end (sometimes a semantic doesn't actually translate well into a given Rust construct after actually thinking about it, even if it makes perfect sense to do it 100% of the time in, say, C, because C doesn't have vtables or closures) or for the codegen backend (see prior discussion about how some architectures must either ignore this request or simply error).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-rust-for-linux Relevant for the Rust-for-Linux project C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants